Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#4176 Add new option to disable SSL Validation in arguments #4617

Merged
merged 8 commits into from
May 20, 2024

Conversation

rohitkrsoni
Copy link
Contributor

This PR includes changes for addition of new option in the argument to disable SSL validation. ALL Tests Passed
image

Regarding new tests: I have doubts over the implementation of the change, if the change is approved, I will be writing the tests and complete any other additional requirements.

@rohitkrsoni
Copy link
Contributor Author

@rohitkrsoni please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contributions!
A couple of initial comments.

src/kiota/Handlers/BaseKiotaCommandHandler.cs Outdated Show resolved Hide resolved
src/kiota/KiotaHost.cs Show resolved Hide resolved
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance you can also run dotnet format to fix the linting errors captured in the CI?

@rohitkrsoni
Copy link
Contributor Author

Any chance you can also run dotnet format to fix the linting errors captured in the CI?

Done, Thanks
image

@pjmagee
Copy link

pjmagee commented May 11, 2024

I agree with this, but it should be for every library. or at least allow to pass in the transport to disable SSL validation, some only allow you to pass in the proxy config but not configure the SSL Cert validation.

@baywet baywet linked an issue May 13, 2024 that may be closed by this pull request
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also sign the CLA please? (see the bot comment)

src/kiota/KiotaHost.cs Show resolved Hide resolved
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes.
We're getting close.
These classes also need to be updated:

Also an entry needs to be added to the changelog

@rohitkrsoni
Copy link
Contributor Author

Thanks for the changes. We're getting close. These classes also need to be updated:

Also an entry needs to be added to the changelog

Hi @baywet, thanks for mentioning the files that needs to be changed but I didn't understand the functionality of Lock Files, could you please brief me about that, thanks

@baywet
Copy link
Member

baywet commented May 17, 2024

It's the file that's generated along with the client code. It effectively contains all the input parameters and a hash of the description. And the update command relies on it when refreshing the clients. This way if somebody generates with this option, and later on refreshes, they won't hit the SSL validation error.
All what needs to be done here is to add this additional property, and use it in the comparison.

@rohitkrsoni
Copy link
Contributor Author

It's the file that's generated along with the client code. It effectively contains all the input parameters and a hash of the description. And the update command relies on it when refreshing the clients. This way if somebody generates with this option, and later on refreshes, they won't hit the SSL validation error. All what needs to be done here is to add this additional property, and use it in the comparison.

Thanks for the clarification, it makes sense now. I have done the changes.

baywet
baywet previously approved these changes May 17, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes! @andrueastman for final review and merge

@rohitkrsoni
Copy link
Contributor Author

rohitkrsoni commented May 17, 2024

Hi @baywet, One more thing, In download command handler, I am setting the value of --disable-ssl-option to Configuration.Download.DisableSSLValidation and we are also using search command in download command. but when checking the configuration in BaseKiotaCommandHandler.cs
https://github.com/rohitkrsoni/kiota/blob/8a0c7fa11c0210fc734689cb69d8b904bb69ce51/src/kiota/Handlers/BaseKiotaCommandHandler.cs#L65-L77
I am only checking for Configuration.Generation.DisableSSLValidation which will not work for download. I am unable to test the download command locally
Getting this error:
image

Will it be fine If I change the check to something like
if (Configuration.Generation.DisableSSLValidation || Configuration.Download.DisableSSLValidation )
Or there are some other option to check the value of DiasbleSSLValidation in all configs?

Sorry for the inconvenience, these things are hitting me at last :(

@rohitkrsoni
Copy link
Contributor Author

Working Screenshots:
Show Command:
image

Generate Command:
image

@baywet
Copy link
Member

baywet commented May 17, 2024

For the search command I don't think we want to be able to disable the SSL validation at this point.
Search talks to GitHub APIs, and the certificate chain should be valid.
Subsequently, download is meant to download one of the search results. And at the moment all those search results will be hosted on GitHub or production services, with valid certificates.
Thanks for the extended testing on this

@andrueastman
Copy link
Member

Thanks for the contribution @rohitkrsoni

@andrueastman andrueastman merged commit 2aa63cf into microsoft:main May 20, 2024
206 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done ✔️
Development

Successfully merging this pull request may close these issues.

Optional parameter to disable SSL verification
4 participants