Skip to content

tls: add local certificate presented flag for mTLS detection #8464

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.

Sign up for GitHub

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

Closed
kyessenov wants to merge 5 commits into from
Closed

tls: add local certificate presented flag for mTLS detection #8464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7500c1e
tls: add local certificate presented flag for mTLS detection
kyessenov Oct 2, 2019
a36b321
typo fix
kyessenov Oct 2, 2019
193c6c1
fix ASAN
kyessenov Oct 2, 2019
25a1057
update to use cert callbacks
kyessenov Oct 4, 2019
51faa62
update tests
kyessenov Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/envoy/ssl/connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ class ConnectionInfo {
**/
virtual bool peerCertificatePresented() const PURE;

/**
* @return bool whether the local certificate is requested on the client by the server. This
* flag is always true on the server.
**/
virtual bool localCertificatePresented() const PURE;

/**
* @return std::string the URIs in the SAN field of the local certificate. Returns {} if there is
* no local certificate, or no SAN field, or no URI.
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/common/expr/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ absl::optional<CelValue> ConnectionWrapper::operator[](CelValue key) const {
auto value = key.StringOrDie().value();
if (value == MTLS) {
return CelValue::CreateBool(info_.downstreamSslConnection() != nullptr &&
info_.downstreamSslConnection()->localCertificatePresented() &&
info_.downstreamSslConnection()->peerCertificatePresented());
} else if (value == RequestedServerName) {
return CelValue::CreateString(info_.requestedServerName());
Expand Down Expand Up @@ -144,6 +145,7 @@ absl::optional<CelValue> UpstreamWrapper::operator[](CelValue key) const {
}
} else if (value == MTLS) {
return CelValue::CreateBool(info_.upstreamSslConnection() != nullptr &&
info_.upstreamSslConnection()->localCertificatePresented() &&
info_.upstreamSslConnection()->peerCertificatePresented());
}

Expand Down
20 changes: 19 additions & 1 deletion source/extensions/transport_sockets/tls/ssl_socket.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ SslSocket::SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)), state_(SocketState::PreHandshake) {
bssl::UniquePtr<SSL> ssl = ctx_->newSsl(transport_socket_options_.get());
ssl_ = ssl.get();
info_ = std::make_shared<SslSocketInfo>(std::move(ssl));
info_ = std::make_shared<SslSocketInfo>(std::move(ssl), state);
if (state == InitialState::Client) {
SSL_set_connect_state(ssl_);
} else {
Expand Down Expand Up @@ -301,11 +301,29 @@ void SslSocket::shutdownSsl() {
}
}

SslSocketInfo::SslSocketInfo(bssl::UniquePtr<SSL> ssl, InitialState state) : ssl_(std::move(ssl)) {
if (state == InitialState::Client) {
SSL_set_cert_cb(
Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora Oct 5, 2019

Choose a reason for hiding this comment

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

It would be more efficient to configure this once in ClientContextImpl using SSL_CTX_set_cert_cb instead of here, when it's called for each connection.

Also, installing it in ClientContextImpl means that there is access to the configuration values, so it might make sense to install this callback only when the client certificate is configured.

Copy link
Copy Markdown
Contributor Author

@kyessenov kyessenov Oct 8, 2019

Choose a reason for hiding this comment

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

I'm not sure how to pass SSL info via context. Can you be more specific about your suggestion?

ssl_.get(),
[](SSL*, void* arg) -> int {
auto info = static_cast<SslSocketInfo*>(arg);
info->local_cert_presented = true;
Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora Oct 5, 2019

Choose a reason for hiding this comment

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

I don't think this is sufficient, since it's not verifying that the client certificate is configured, and I think that this callback will be called even if it isn't (I might be wrong, so it's worth double-checking that).

This should work better:
info->local_cert_presented = (SSL_get_certificate(ssl) != nullptr);

Ideally, local_cert_presented should be set to true only if the issuer of the client certificate matches one of the CAs that server requested a certificate for (using SSL_get_client_CA_list and SSL_get0_certificate_types), but we're not checking for that before sending the certificate, and server is going to close the connection if they don't match anyway, so it might be an overkill if we want to use this only for telemetry.

Copy link
Copy Markdown
Contributor Author

@kyessenov kyessenov Oct 7, 2019

Choose a reason for hiding this comment

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

We assume that handshake completes successfully. That means all those checks (there is a client cert, and CA trust chain, and other things) are already done. Why do we need another set of checks that duplicate the handshake?

return 1;
},
this);
} else {
ASSERT(state == InitialState::Server);
local_cert_presented = true;
}
}

bool SslSocketInfo::peerCertificatePresented() const {
bssl::UniquePtr<X509> cert(SSL_get_peer_certificate(ssl_.get()));
return cert != nullptr;
}

bool SslSocketInfo::localCertificatePresented() const { return local_cert_presented; }

std::vector<std::string> SslSocketInfo::uriSanLocalCertificate() const {
if (!cached_uri_san_local_certificate_.empty()) {
return cached_uri_san_local_certificate_;
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/transport_sockets/tls/ssl_socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,11 @@ enum class SocketState { PreHandshake, HandshakeInProgress, HandshakeComplete, S

class SslSocketInfo : public Envoy::Ssl::ConnectionInfo {
public:
SslSocketInfo(bssl::UniquePtr<SSL> ssl) : ssl_(std::move(ssl)) {}
SslSocketInfo(bssl::UniquePtr<SSL> ssl, InitialState state);

// Ssl::ConnectionInfo
bool peerCertificatePresented() const override;
bool localCertificatePresented() const override;
std::vector<std::string> uriSanLocalCertificate() const override;
const std::string& sha256PeerCertificateDigest() const override;
const std::string& serialNumberPeerCertificate() const override;
Expand Down Expand Up @@ -83,6 +84,7 @@ class SslSocketInfo : public Envoy::Ssl::ConnectionInfo {
mutable std::vector<std::string> cached_dns_san_local_certificate_;
mutable std::string cached_session_id_;
mutable std::string cached_tls_version_;
bool local_cert_presented = false;
};

class SslSocket : public Network::TransportSocket,
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/common/expr/context_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,9 @@ TEST(Context, ConnectionAttributes) {
EXPECT_CALL(info, upstreamHost()).WillRepeatedly(Return(upstream_host));
EXPECT_CALL(info, requestedServerName()).WillRepeatedly(ReturnRef(sni_name));
EXPECT_CALL(*downstream_ssl_info, peerCertificatePresented()).WillRepeatedly(Return(true));
EXPECT_CALL(*downstream_ssl_info, localCertificatePresented()).WillRepeatedly(Return(true));
EXPECT_CALL(*upstream_ssl_info, peerCertificatePresented()).WillRepeatedly(Return(true));
EXPECT_CALL(*upstream_ssl_info, localCertificatePresented()).WillRepeatedly(Return(true));
const std::string tls_version = "TLSv1";
EXPECT_CALL(*downstream_ssl_info, tlsVersion()).WillRepeatedly(ReturnRef(tls_version));
EXPECT_CALL(*upstream_host, address()).WillRepeatedly(Return(upstream_address));
Expand Down
47 changes: 39 additions & 8 deletions test/extensions/transport_sockets/tls/ssl_socket_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class TestUtilOptions : public TestUtilOptionsBase {
bool expect_success, Network::Address::IpVersion version)
: TestUtilOptionsBase(expect_success, version), client_ctx_yaml_(client_ctx_yaml),
server_ctx_yaml_(server_ctx_yaml), expect_no_cert_(false), expect_no_cert_chain_(false),
expect_private_key_method_(false),
expect_local_cert_(false), expect_private_key_method_(false),
expected_server_close_event_(Network::ConnectionEvent::RemoteClose) {
if (expect_success) {
setExpectedServerStats("ssl.handshake");
Expand Down Expand Up @@ -131,6 +131,13 @@ class TestUtilOptions : public TestUtilOptionsBase {
return *this;
}

bool expectLocalCertPresented() const { return expect_local_cert_; }

TestUtilOptions& setExpectLocalCertPresented() {
expect_local_cert_ = true;
return *this;
}

TestUtilOptions& setExpectedClientCertUri(const std::string& expected_client_cert_uri) {
TestUtilOptionsBase::setExpectedClientCertUri(expected_client_cert_uri);
return *this;
Expand Down Expand Up @@ -230,6 +237,7 @@ class TestUtilOptions : public TestUtilOptionsBase {

bool expect_no_cert_;
bool expect_no_cert_chain_;
bool expect_local_cert_;
bool expect_private_key_method_;
Network::ConnectionEvent expected_server_close_event_;
std::string expected_digest_;
Expand Down Expand Up @@ -343,6 +351,7 @@ void testUtil(const TestUtilOptions& options) {
server_connection->ssl()->subjectPeerCertificate());
}
if (!options.expectedLocalSubject().empty()) {
EXPECT_TRUE(server_connection->ssl()->localCertificatePresented());
EXPECT_EQ(options.expectedLocalSubject(),
server_connection->ssl()->subjectLocalCertificate());
}
Expand Down Expand Up @@ -381,10 +390,18 @@ void testUtil(const TestUtilOptions& options) {
EXPECT_EQ(EMPTY_STRING, server_connection->ssl()->subjectPeerCertificate());
EXPECT_EQ(std::vector<std::string>{}, server_connection->ssl()->dnsSansPeerCertificate());
}

if (options.expectNoCertChain()) {
EXPECT_EQ(EMPTY_STRING,
server_connection->ssl()->urlEncodedPemEncodedPeerCertificateChain());
}

if (options.expectLocalCertPresented()) {
EXPECT_TRUE(client_connection->ssl()->localCertificatePresented());
} else {
EXPECT_FALSE(client_connection->ssl()->localCertificatePresented());
}

// By default, the session is not created with session resumption. The
// client should see a session ID but the server should not.
EXPECT_EQ(EMPTY_STRING, server_connection->ssl()->sessionId());
Expand Down Expand Up @@ -791,6 +808,7 @@ TEST_P(SslSocketTest, GetCertDigest) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedDigest(TEST_NO_SAN_CERT_HASH)
.setExpectLocalCertPresented()
.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL));
}

Expand Down Expand Up @@ -862,6 +880,7 @@ TEST_P(SslSocketTest, GetCertDigestServerCertWithIntermediateCA) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedDigest(TEST_NO_SAN_CERT_HASH)
.setExpectLocalCertPresented()
.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL));
}

Expand Down Expand Up @@ -889,6 +908,7 @@ TEST_P(SslSocketTest, GetCertDigestServerCertWithoutCommonName) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedDigest(TEST_NO_SAN_CERT_HASH)
.setExpectLocalCertPresented()
.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL));
}

Expand Down Expand Up @@ -917,6 +937,7 @@ TEST_P(SslSocketTest, GetUriWithUriSan) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedClientCertUri("spiffe://lyft.com/test-team")
.setExpectLocalCertPresented()
.setExpectedSerialNumber(TEST_SAN_URI_CERT_SERIAL));
}

Expand Down Expand Up @@ -990,7 +1011,8 @@ TEST_P(SslSocketTest, GetNoUriWithDnsSan) {

// The SAN field only has DNS, expect "" for uriSanPeerCertificate().
TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedSerialNumber(TEST_SAN_DNS_CERT_SERIAL));
testUtil(
test_options.setExpectedSerialNumber(TEST_SAN_DNS_CERT_SERIAL).setExpectLocalCertPresented());
}

TEST_P(SslSocketTest, NoCert) {
Expand Down Expand Up @@ -1070,6 +1092,7 @@ TEST_P(SslSocketTest, GetUriWithLocalUriSan) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedLocalUri("spiffe://lyft.com/test-team")
.setExpectLocalCertPresented()
.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL));
}

Expand Down Expand Up @@ -1098,6 +1121,7 @@ TEST_P(SslSocketTest, GetSubjectsWithBothCerts) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL)
.setExpectLocalCertPresented()
.setExpectedPeerIssuer(
"CN=Test CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US")
.setExpectedPeerSubject(
Expand Down Expand Up @@ -1134,6 +1158,7 @@ TEST_P(SslSocketTest, GetPeerCert) {
TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(
"{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/no_san_cert.pem"));
testUtil(test_options.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL)
.setExpectLocalCertPresented()
.setExpectedPeerIssuer(
"CN=Test CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US")
.setExpectedPeerSubject(
Expand Down Expand Up @@ -1172,6 +1197,7 @@ TEST_P(SslSocketTest, GetPeerCertChain) {
"{{ test_rundir "
"}}/test/extensions/transport_sockets/tls/test_data/no_san_chain.pem"));
testUtil(test_options.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL)
.setExpectLocalCertPresented()
.setExpectedPeerCertChain(expected_peer_cert_chain));
}

Expand Down Expand Up @@ -1199,6 +1225,7 @@ TEST_P(SslSocketTest, GetIssueExpireTimesPeerCert) {
)EOF";
TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedSerialNumber(TEST_NO_SAN_CERT_SERIAL)
.setExpectLocalCertPresented()
.setExpectedValidFromTimePeerCert(TEST_NO_SAN_CERT_NOT_BEFORE)
.setExpectedExpirationTimePeerCert(TEST_NO_SAN_CERT_NOT_AFTER));
}
Expand Down Expand Up @@ -1421,6 +1448,7 @@ TEST_P(SslSocketTest, ClientCertificateHashVerification) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedClientCertUri("spiffe://lyft.com/test-team")
.setExpectLocalCertPresented()
.setExpectedSerialNumber(TEST_SAN_URI_CERT_SERIAL));
}

Expand All @@ -1447,6 +1475,7 @@ TEST_P(SslSocketTest, ClientCertificateHashVerificationNoCA) {

TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam());
testUtil(test_options.setExpectedClientCertUri("spiffe://lyft.com/test-team")
.setExpectLocalCertPresented()
.setExpectedSerialNumber(TEST_SAN_URI_CERT_SERIAL));
}

Expand Down Expand Up @@ -3694,7 +3723,8 @@ TEST_P(SslSocketTest, RevokedCertificate) {

TestUtilOptions successful_test_options(successful_client_ctx_yaml, server_ctx_yaml, true,
GetParam());
testUtil(successful_test_options.setExpectedSerialNumber(TEST_SAN_DNS2_CERT_SERIAL));
testUtil(successful_test_options.setExpectedSerialNumber(TEST_SAN_DNS2_CERT_SERIAL)
.setExpectLocalCertPresented());
}

TEST_P(SslSocketTest, RevokedCertificateCRLInTrustedCA) {
Expand Down Expand Up @@ -3735,7 +3765,8 @@ TEST_P(SslSocketTest, RevokedCertificateCRLInTrustedCA) {
)EOF";
TestUtilOptions successful_test_options(successful_client_ctx_yaml, server_ctx_yaml, true,
GetParam());
testUtil(successful_test_options.setExpectedSerialNumber(TEST_SAN_DNS2_CERT_SERIAL));
testUtil(successful_test_options.setExpectedSerialNumber(TEST_SAN_DNS2_CERT_SERIAL)
.setExpectLocalCertPresented());
}

TEST_P(SslSocketTest, GetRequestedServerName) {
Expand Down Expand Up @@ -4220,7 +4251,7 @@ TEST_P(SslSocketTest, RsaPrivateKeyProviderAsyncSignSuccess) {

TestUtilOptions successful_test_options(successful_client_ctx_yaml, server_ctx_yaml, true,
GetParam());
testUtil(successful_test_options.setPrivateKeyMethodExpected(true));
testUtil(successful_test_options.setPrivateKeyMethodExpected(true).setExpectLocalCertPresented());
}

// Test asynchronous decryption (RSA).
Expand Down Expand Up @@ -4252,7 +4283,7 @@ TEST_P(SslSocketTest, RsaPrivateKeyProviderAsyncDecryptSuccess) {

TestUtilOptions successful_test_options(successful_client_ctx_yaml, server_ctx_yaml, true,
GetParam());
testUtil(successful_test_options.setPrivateKeyMethodExpected(true));
testUtil(successful_test_options.setPrivateKeyMethodExpected(true).setExpectLocalCertPresented());
}

// Test synchronous signing (ECDHE).
Expand Down Expand Up @@ -4284,7 +4315,7 @@ TEST_P(SslSocketTest, RsaPrivateKeyProviderSyncSignSuccess) {

TestUtilOptions successful_test_options(successful_client_ctx_yaml, server_ctx_yaml, true,
GetParam());
testUtil(successful_test_options.setPrivateKeyMethodExpected(true));
testUtil(successful_test_options.setPrivateKeyMethodExpected(true).setExpectLocalCertPresented());
}

// Test synchronous decryption (RSA).
Expand Down Expand Up @@ -4316,7 +4347,7 @@ TEST_P(SslSocketTest, RsaPrivateKeyProviderSyncDecryptSuccess) {

TestUtilOptions successful_test_options(successful_client_ctx_yaml, server_ctx_yaml, true,
GetParam());
testUtil(successful_test_options.setPrivateKeyMethodExpected(true));
testUtil(successful_test_options.setPrivateKeyMethodExpected(true).setExpectLocalCertPresented());
}

// Test asynchronous signing (ECDHE) failure (invalid signature).
Expand Down
1 change: 1 addition & 0 deletions test/mocks/ssl/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class MockConnectionInfo : public ConnectionInfo {
~MockConnectionInfo() override;

MOCK_CONST_METHOD0(peerCertificatePresented, bool());
MOCK_CONST_METHOD0(localCertificatePresented, bool());
MOCK_CONST_METHOD0(uriSanLocalCertificate, std::vector<std::string>());
MOCK_CONST_METHOD0(sha256PeerCertificateDigest, const std::string&());
MOCK_CONST_METHOD0(serialNumberPeerCertificate, const std::string&());
Expand Down