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

SSL example for HttpUrlConnection #861

Merged
merged 8 commits into from Feb 22, 2021
Merged

SSL example for HttpUrlConnection #861

merged 8 commits into from Feb 22, 2021

Conversation

kubinio123
Copy link
Contributor

Before submitting pull request:

  • Check if the project compiles by running sbt compile
  • Verify docs compilation by running sbt compileDocs
  • Check if tests pass by running sbt test
  • Format code by running sbt scalafmt

docs/conf/ssl.md Outdated

Common requirement for handling SSL is creating `SSLContext`. It's required by several backends.

Example assumes that you have your client key store in `.p12` format and the server certificate imported to your trust store.
Copy link
Member

Choose a reason for hiding this comment

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

Looks great! Couple of questions:

  • what is described here is importing certificates for a server, and hence implementing one way SSL, right? If so, it would be good to explicitly mention that - I suspect people might look for docs on "how to implement one way SSL" or "how to implement mutual SSL"
  • sometimes you need to disable SSL - only during development of course ;) - I think a section on that would be helpful as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • It's actually a mutual SSL example since we have both client key store (.p12 file) and trust store with server certificate imported. So server identifies client and vice versa. I can add an example without trust store (one way)
  • I could add a code sample on how to disable SSL

Copy link
Member

Choose a reason for hiding this comment

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

great :) I think it would be best to have sections on one-way / mutual / disabled SSL - I'm guessing that's what people will be looking for in the docs in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but i didn't add explicit section about disabled SSL because that depends on server config as well.
On the client side we can disable validation of server certificates and that's included in "One way SSL" section.

Copy link
Member

Choose a reason for hiding this comment

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

looks good, thank you!

docs/conf/ssl.md Outdated
`openssl pkcs12 -export -inkey your_key.pem -in your_cert.pem -out your_cert.p12`

Sample code might look like this:
```scala
Copy link
Member

Choose a reason for hiding this comment

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

can we enable mdoc here? (using scala mdoc:silent)

that would verify that the code compiles (at least :) ) see the readme how to run docs generation localy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can use mdoc:compile-only here but variables are out of scope for mdoc in further code samples so I haven't used it at first

Copy link
Member

Choose a reason for hiding this comment

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

According to the docs: https://scalameta.org/mdoc/docs/modifiers.html, mdoc:silent accumulates the definitions. Would be good to enable it as much as possible, it really helps in keeping the docs up-to-date :)

@adamw adamw merged commit 97e98c2 into master Feb 22, 2021
@mergify mergify bot deleted the issue/114 branch February 22, 2021 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants