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

Use full certificate chain for webhook #515

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Conversation

mcb30
Copy link
Contributor

@mcb30 mcb30 commented Mar 29, 2020

Pull Request (PR) description

Split from #511

The webhook service is currently limited to using a certificate
directly issued by a trusted CA; it will silently ignore any
intermediate certificates that are present in the certificate file.

The currently released versions of the Ruby OpenSSL libraries do not
provide any clean way to load a certificate chain from a file. We
therefore split the file using the BEGIN/END markers as per RFC 7468,
and construct the certificate chain directly.

No tests are extended to cover this enhancement, since there is no
existing test coverage for the use of HTTPS certificates by the
webhook. All current tests use plain HTTP via http://localhost:8088.

This Pull Request (PR) fixes the following issues

Fixes #510

@vchepkov
Copy link
Contributor

Wouldn't it be better just provide intermediate ca in a separate file?

@mcb30
Copy link
Contributor Author

mcb30 commented Mar 30, 2020

Wouldn't it be better just provide intermediate ca in a separate file?

There may be multiple intermediate certificates, so the PEM splitting logic would still be required.

I chose to use the full-chain approach because it minimises the changes to the existing codebase and is backwards-compatible with existing configurations: if an existing configuration happens to switch to a cert file that now includes an intermediate cert then it will work with no configuration changes required.

@vchepkov I can rework the patch to require the intermediate certs to be held in a separate file (and hence separate configuration parameters, etc) if you prefer, but it will necessarily be a much more invasive patch.

@vchepkov
Copy link
Contributor

I was just curios and yes, multiple intermediate certificates would have the same issue.
Since webhook interface is a very specialized service and configured/called in point-to-point manner only by git hooks, this fix is only necessary if there is no option to turn off certificate verification. I imagine code will fail if certificate and key are kept in the same file (as some folks do). Why don't you split using -----BEGIN CERTIFICATE-----.+-----END CERTIFICATE----- ?

@mcb30
Copy link
Contributor Author

mcb30 commented Mar 30, 2020

Since webhook interface is a very specialized service and configured/called in point-to-point manner only by git hooks, this fix is only necessary if there is no option to turn off certificate verification.

In our use case, the webhook will be called by a git management service in a different administrative domain (e.g. GitHub), so there is no way to disable certificate verification.

I imagine code will fail if certificate and key are kept in the same file (as some folks do). Why don't you split using -----BEGIN CERTIFICATE-----.+-----END CERTIFICATE----- ?

Can do. RFC 7468 mentions some existing non-standard implementations that use "BEGIN X.509 CERTIFICATE". How about a regex of /-----BEGIN.*?CERTIFICATE-----.*?-----END.*?CERTIFICATE-----/m?

@vchepkov
Copy link
Contributor

I think it should be .+?, but yes, otherwise you can end up with chain having only the key and it would fail

@mcb30
Copy link
Contributor Author

mcb30 commented Mar 30, 2020

I think it should be .+?, but yes, otherwise you can end up with chain having only the key and it would fail

Thanks. Will modify the code and update the PR after checking that it still works in my test deployment.

The webhook service is currently limited to using a certificate
directly issued by a trusted CA; it will silently ignore any
intermediate certificates that are present in the certificate file.

The currently released versions of the Ruby OpenSSL libraries do not
provide any clean way to load a certificate chain from a file.  We
therefore split the file using the BEGIN/END markers as per RFC 7468,
and construct the certificate chain directly.

No tests are extended to cover this enhancement, since there is no
existing test coverage for the use of HTTPS certificates by the
webhook.  All current tests use plain HTTP via http://localhost:8088.

Fixes voxpupuli#510

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
@mcb30
Copy link
Contributor Author

mcb30 commented Mar 31, 2020

@alexjfisher Are there any changes you'd like made, or can this be merged? Thanks.

@alexjfisher alexjfisher merged commit bacf4d5 into voxpupuli:master Mar 31, 2020
@alexjfisher
Copy link
Member

@mcb30 Thanks!

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.

webhook does not support the use of intermediate certificates
3 participants