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

Optional parameter to disable SSL verification #4176

Closed
thgla opened this issue Feb 15, 2024 · 16 comments · Fixed by #4617
Closed

Optional parameter to disable SSL verification #4176

thgla opened this issue Feb 15, 2024 · 16 comments · Fixed by #4617
Assignees
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. good first issue Good for newcomers help wanted Issue caused by core project dependency modules or library WIP
Milestone

Comments

@thgla
Copy link

thgla commented Feb 15, 2024

It would be nice to have an optional parameter for disabling SSL verification. Currently, generating the API client from an HTTPS source with an untrusted certificate, will lead to an error:

crit: Kiota.Builder.KiotaBuilder[0]
      error generating the client: Could not download the file at https://localhost:9003/swagger/v1/swagger.json, reason: The SSL connection could not be established, see inner exception.

The workaround I've been using so far: Downloading the swagger.json manually, and then scaffolding from the file instead. This is rather inconvenient.

The optional parameter could be like this:

kiota generate --openapi https://localhost:9003/swagger/v1/swagger.json --language csharp --disable-ssl-validation

There are several reasons why this would be useful:

  • When I'm scaffolding from a development environment, the API is very likely to be using a self-signed certificate which won't be trusted by the system
  • Lots of companies use SSL proxies in their internal networks that might interfere as well
@baywet baywet added enhancement New feature or request help wanted Issue caused by core project dependency modules or library generator Issues or improvements relater to generation capabilities. labels Feb 15, 2024
@baywet baywet added this to the Backlog milestone Feb 15, 2024
@baywet
Copy link
Member

baywet commented Feb 15, 2024

Hi @thgla
Thanks for using kiota and for reaching out.
The only place where we instantiate the http client is here

internal static readonly HttpClient httpClient = new();

With a custom SSL validator, we should be able to suppress errors for localhost, 127.0.0.1, [::1]

Is this something you'd be willing to submit a pull request for?

@baywet baywet added the good first issue Good for newcomers label Feb 15, 2024
@thgla
Copy link
Author

thgla commented Feb 16, 2024

I can take a stab at this, might take a bit though. But after having glanced at the code base now, this doesn't seem too difficult

@baywet
Copy link
Member

baywet commented Mar 20, 2024

@thgla is this still something you're planning to work on?

@thgla
Copy link
Author

thgla commented Mar 21, 2024

Yeah, unless someone else will do it - I'm just a bit busy at the moment with other projects

@rohitkrsoni
Copy link
Contributor

Hi, I would like to work on this issue, can I get this assigned and have some insights of where to start from, thanks

@baywet baywet assigned rohitkrsoni and unassigned thgla May 6, 2024
@baywet
Copy link
Member

baywet commented May 6, 2024

Hi @rohitkrsoni
Thanks for volunteering!
You should have all the details you need in this answer
Let us know if you have additional questions.

@rohitkrsoni
Copy link
Contributor

rohitkrsoni commented May 6, 2024

Thanks for assigning this issue to me, I am very willing and excited to contribute to this project. I cloned the repo and tried to run the project in VS but I am getting error in the command line.
image
I followed Microsoft documentation to setup my local provided here https://learn.microsoft.com/en-us/openapi/kiota/install?tabs=bash#build-from-source and https://github.com/microsoft/kiota/blob/main/CONTRIBUTING.md (All tests passed)

I tried using the bash command and run the kiota.exe manually but a cmd window opened and closed instantly
Is there something I am missing?

One more thing (This might be a silly one) For some reason I can only see few files in VS Solution Explorer, probably because I am using solution view and I need to change to folder view?
image

Thanks

@andrueastman
Copy link
Member

Hey @rohitkrsoni,

Looks like the launch config for Visual Studio isn't setup correctly on the main branch and still uses configuration of an older version of the library. (This has been setup though for VS code ).

What you would probably need to do is update the launch settings here

"commandLineArgs": "--openapi C:\\src\\msgraph-sdk-powershell\\openApiDocs\\v1.0\\mail.yml -o C:\\Users\\darrmi\\source\\github\\darrelmiller\\OpenApiClient\\Generated -c GraphClient --loglevel Information"

To be something like this so that launch runs correctly. (It would be great if you included the fix in the PR as well)

"generate --openapi https://raw.githubusercontent.com/microsoftgraph/msgraph-sdk-powershell/dev/openApiDocs/v1.0/Mail.yml -o {output path for generated files} -c GraphClient --log-level Information -l CSharp"

One more thing (This might be a silly one) For some reason I can only see few files in VS Solution Explorer, probably because I am using solution view and I need to change to folder view?

Most of the files should be under the kiota and Kiota.Builder projects. So, if you expand those you should see them in a fashion similar the folder structure.

rohitkrsoni added a commit to rohitkrsoni/kiota that referenced this issue May 7, 2024
@rohitkrsoni
Copy link
Contributor

Thanks @andrueastman, for the clarification

Team, I have added a PR for the change, here is the link #4617. Please review it and let me know if any other changes are required, thanks

rohitkrsoni added a commit to rohitkrsoni/kiota that referenced this issue May 8, 2024
@darrelmiller
Copy link
Member

darrelmiller commented May 8, 2024

Hey folks, can you help me understand the requirements for this feature a little bit better. For context, security is an even more sensitive issue these days and a feature that disables SSL validation seems like an opportunity for something to go wrong.

I've seen two scenarios discussed so far where this feature might help:

  • inner dev loop where localhost server is exposing an https endpoint
  • Internal company proxy gateway that uses SSL

For the first scenario, I would have thought the two easier solutions would be

  • create a self-signed certificate
  • expose the service as http only while in dev mode

For the second case, I don't really understand how it would happen that a company would set up an internal proxy that doesn't have a valid SSL configuration. That would make using a web browser within the company pretty much impossible. I've seen internal proxies use http on the local network and then HTTPS outside, if they don't have their own internal CA, but I don't understand how someone could setup internal SSL without a valid cert.

I'm fully prepared to accept I am misinformed. I just would like to get more clarification on this feature before we ship a security "foot gun". Thanks.

/cc @rohitkrsoni @thgla

@thgla
Copy link
Author

thgla commented May 8, 2024

@darrelmiller: In my mentioned scenario, the localhost API is using a self-signed certificate, but these aren't trusted by default. So I would have to establish a trust first before I can scaffold, which is possible.. but then I might as well just do the workaround I mentioned initally by downloading the swagger.json manually. The other scenario is not as likely I agree, but I have been in a situtation where I wanted to scaffold something while in a WSL/Ubuntu environment, which doesn't share the same trust store as the host Windows system. Bypassing validation entirely is admittedly not the best solution, it's preferable to just establish trust correctly, but for these one-off actions where I know I'm on the company network anyway it doesn't feel necessary to me.

So my thinking was that it's reasonable to assume a developer who explicitly adds --disable-ssl-verification understands the implications and risks of that, but just wants to "get it done" without needing to deal with workarounds or trying to establish a trusted chain. It's not that uncommon to see options for disabling SSL validation in developer tools, Git has one as well (git -c http.sslVerify=false ...)

@baywet
Copy link
Member

baywet commented May 8, 2024

@thgla Thanks for the additional information. In that scenario of talking from WSL to host, or vice versa, is the callback URL still localhost? (or a loopback address)

@darrelmiller
Copy link
Member

I prefer the explicitness of the flag over a the hidden effect of an environment variable.

@baywet
Copy link
Member

baywet commented May 9, 2024

alright, trying to drive this to conclusion so we can do something with the PR here.
@darrelmiller was the scenario from @thgla and the demonstration with git convincing enough for you to accept that change?

@baywet baywet linked a pull request May 13, 2024 that will close this issue
@darrelmiller
Copy link
Member

@baywet Yeah, sorry, I should have been more explicit. The fact that git has this option was probably the most convincing argument for me. I am ok with us implementing it as a flag.

rohitkrsoni added a commit to rohitkrsoni/kiota that referenced this issue May 17, 2024
andrueastman added a commit that referenced this issue May 20, 2024
#4176 Add new option to disable SSL Validation in arguments
@thgla
Copy link
Author

thgla commented May 21, 2024

Awesome, thanks to @rohitkrsoni for the implementation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. good first issue Good for newcomers help wanted Issue caused by core project dependency modules or library WIP
Projects
Status: Done ✔️
Development

Successfully merging a pull request may close this issue.

5 participants