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

Check certificate SAN IP address when appropriate #96

Merged
merged 8 commits into from
May 28, 2021

Conversation

Firgeis
Copy link
Contributor

@Firgeis Firgeis commented May 8, 2021

In this PR:

  • Fixes a bug that fails the certificate check when host-name is an IP address
  • Improves check logic to search in certificate SAN for the IP address when appropriate
  • Adds test for the above

Requires the following commit from dependency ocaml-ssl: Firgeis/ocaml-ssl@e6430aa

lib/openssl.ml Outdated Show resolved Hide resolved
@anmonteiro
Copy link
Owner

Thanks for working on this. The spirit of the change makes sense to me, and the implementation looks straightforward. The only minor concern I have is a hard dependency on ipaddr, even if it's fairly lightweight. Did you consider other alternatives instead of ipaddr? Perhaps something in the uri API?

@Firgeis
Copy link
Contributor Author

Firgeis commented May 8, 2021

Thanks for working on this. The spirit of the change makes sense to me, and the implementation looks straightforward. The only minor concern I have is a hard dependency on ipaddr, even if it's fairly lightweight. Did you consider other alternatives instead of ipaddr? Perhaps something in the uri API?

I really did look into the URI API, but I did not find anything relevant to parsing IP addresses. Is there something you suggest?

@Firgeis Firgeis requested a review from anmonteiro May 11, 2021 16:17
@anmonteiro anmonteiro changed the title Check certificate SAN IP address when appropiate Check certificate SAN IP address when appropriate May 26, 2021
@anmonteiro
Copy link
Owner

  • I think this makes sense
  • the ipaddr library dependency doesn't seem too heavy
  • tests are flaky

so I'm just gonna go ahead and merge. Thanks so much!

@anmonteiro anmonteiro merged commit abab9cb into anmonteiro:master May 28, 2021
anmonteiro added a commit that referenced this pull request May 28, 2021
anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Sep 6, 2024
CHANGES:

- Improve certificate checking and authentication
  ([anmonteiro/piaf#93](anmonteiro/piaf#93)) -
  [@Firgeis](https://github.com/Firgeis)
- Check certificate SAN IP address when appropriate
  ([anmonteiro/piaf#96](anmonteiro/piaf#96)) -
  [@Firgeis](https://github.com/Firgeis)
- Close the file descriptor when failing to open a connection
  ([anmonteiro/piaf#97](anmonteiro/piaf#97)) -
  [@EduardoRFS](https://github.com/EduardoRFS)
- Yield to other threads when reading a message body. This improves fairness
  for large message bodies
  ([anmonteiro/piaf#100](anmonteiro/piaf#100))
- Add error handling to `Response.of_file`
  ([anmonteiro/piaf#103](anmonteiro/piaf#103))
- Add `Client.send` which sends a `Request.t`
  ([anmonteiro/piaf#110](anmonteiro/piaf#110))
- openssl: set the client verify callback
  ([anmonteiro/piaf#112](anmonteiro/piaf#112))
- Piaf.Response: add `or_internal_error`
  ([anmonteiro/piaf#120](anmonteiro/piaf#120))
- Piaf.Response: Add `Body.sendfile` and `Response.sendfile`
  ([anmonteiro/piaf#124](anmonteiro/piaf#124))
- Piaf.Config: Add `config.flush_headers_immediately`
  ([anmonteiro/piaf#125](anmonteiro/piaf#125))
- Piaf.Server: Add `config.shutdown_timeout` to wait before shutting down the
  Piaf server ([anmonteiro/piaf#174](anmonteiro/piaf#174))
- Websocket support ([anmonteiro/piaf#139](anmonteiro/piaf#139))
- Multicore support ([anmonteiro/piaf#151](anmonteiro/piaf#151))
- Allow binding to UNIX domain socket
  ([anmonteiro/piaf#161](anmonteiro/piaf#161))
- Don't send invalid HTTP/2 headers
  ([anmonteiro/piaf#197](anmonteiro/piaf#197))
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.

2 participants