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

Add support for custom certificate verification callbacks #7317

Merged

Conversation

vekterli
Copy link
Member

@bjorncs please review. You are the lucky recipient of this bundle of crypto joy due to the sheer amount of key/X.509 meddling it involves. Note that the X.509 building and keygen code is currently only built and used by tests. It is intended to be upgraded to production library code later, but that will require a lot of domain specific tests to be written first.
@havardpe FYI

Manually tested with OpenSSL versions 1.0.1e, 1.0.2k, 1.1.0u and 1.1.1.

Callback is specified as part of TransportSecurityOptions and will default
to a callback accepting all pre-verified certificates if not given.
Callback is provided with certificate subject Common Name and
DNS Subject Alternate Name entries.

Specified as part of `TransportSecurityOptions` and will default
to a callback accepting all pre-verified certificates if not given.
Callback is provided with certificate subject Common Name and
DNS Subject Alternate Name entries.
@vekterli
Copy link
Member Author

This PR is part of #7219

Copy link
Member

@bjorncs bjorncs left a comment

Choose a reason for hiding this comment

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

See my comments. Approving as the comments are focusing on the production use case..


struct SubjectInfo {
DistinguishedName dn;
std::vector<vespalib::string> subject_alt_names;
Copy link
Member

Choose a reason for hiding this comment

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

The SANs should contain the type (DNS, IP, etc) to in case other SAN types are required for a production use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do when this code is refactored out of test module (or IP SANs are added, whichever comes first)

virtual ~CertificateVerificationCallback() = default;
// Return true iff the peer credentials pass verification, false otherwise.
// Must be thread safe.
virtual bool verify(const PeerCredentials& peer_creds) const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding the remote IP address (or similar) as well. That will allow for the typical browser verification (CN/SAN matches DNS of remote ip).

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely agree we want this, but the context in which the callback is invoked unfortunately does not contain any networking details. Will likely want the networking layer to explicitly pass down peer information when crypto sockets/codecs are created. Then this information can in turn be passed on to the verification callback.

// or the empty string if no CN is given (or if the CN is curiously empty).
vespalib::string common_name;
// 0-n DNS SAN entries. Note: "DNS:" prefix is not present in strings.
std::vector<vespalib::string> dns_sans;
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment on SANs.

Copy link
Member Author

Choose a reason for hiding this comment

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

IPADDR SAN support Coming Soon(tm).

@vekterli vekterli merged commit 63ab626 into master Oct 16, 2018
@vekterli vekterli deleted the vekterli/custom-tls-certificate-verification-callback branch October 16, 2018 14:41
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