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] Add ability to set verification flags #2490

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

Bonias
Copy link
Contributor

@Bonias Bonias commented Nov 23, 2020

Description

Add ability to set OpenSSL verification flags (https://www.openssl.org/docs/manmaster/man3/X509_VERIFY_PARAM_set_hostflags.html#VERIFICATION-FLAGS).

We had a need to set X509_V_FLAG_PARTIAL_CHAIN flag. Most major TLS libraries behaves like this by default but not OpenSSL (openssl/openssl#7871). Adding ability to set any verification flag allows us to set X509_V_FLAG_PARTIAL_CHAIN flag. I think it may be useful for others too.

I implemented this for MRI only as I don't know how to do this for JRuby (and if this is even required?).

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • (almost) My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

README.md Outdated Show resolved Hide resolved
lib/puma/dsl.rb Outdated Show resolved Hide resolved
@Bonias Bonias force-pushed the ssl-allow-to-set-verification-flags branch from b32249b to e46db09 Compare November 24, 2020 10:34
@MSP-Greg
Copy link
Member

@Bonias

Not that familiar with these flags, or how and when they may be set. In mini_ssl.c, might it be better to change:

    param = X509_VERIFY_PARAM_new();

to something like:

    param = SSL_CTX_get0_param(ctx);

@Bonias Bonias force-pushed the ssl-allow-to-set-verification-flags branch from e46db09 to 40a6efa Compare November 26, 2020 11:04
@Bonias
Copy link
Contributor Author

Bonias commented Nov 26, 2020

Thanks @MSP-Greg! I changed it to SSL_CTX_get0_param

@nateberkopec nateberkopec added feature ssl waiting-for-review Waiting on review from anyone labels Nov 27, 2020
@MSP-Greg
Copy link
Member

MSP-Greg commented Dec 3, 2020

@Bonias

Well, I found out what one can't do with git...

I rebased this, adjusted dsl.rb, then hit a compile issue, fixed that, then decided that, rather than repeating &verification_flags= in the query string, that concatenating the strings would be a lot easier to look at...

I don't know what you think, but the patch is at:

https://github.com/MSP-Greg/puma/commit/4d063d982c5.patch

If you rebase, then replace the PR commit with the above, it should be good to merge, and CI did pass...

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Dec 7, 2020
@Bonias Bonias force-pushed the ssl-allow-to-set-verification-flags branch from 40a6efa to 555e4a8 Compare December 7, 2020 23:12
@Bonias
Copy link
Contributor Author

Bonias commented Dec 7, 2020

I rebased this. I also updated README and History files. I hope all is good now.

@MSP-Greg MSP-Greg merged commit d2f9d6d into puma:master Dec 7, 2020
@MSP-Greg
Copy link
Member

MSP-Greg commented Dec 7, 2020

@Bonias

Thank you. Sorry for the trouble, the changes I made to DSL got merged first, so this got messy.

The DSL changes allow access to the ssl bind string, which makes CI a bit easier...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ssl waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants