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.

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

cleanup: make TLS getters return const std::string& #8192

Merged
merged 1 commit into from
Sep 10, 2019
+149 −83
Merged

cleanup: make TLS getters return const std::string& #8192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions include/envoy/ssl/connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class ConnectionInfo {
* @return std::string the subject field of the local certificate in RFC 2253 format. Returns ""
* if there is no local certificate, or no subject.
**/
virtual std::string subjectLocalCertificate() const PURE;
virtual const std::string& subjectLocalCertificate() const PURE;

/**
* @return std::string the SHA256 digest of the peer certificate. Returns "" if there is no peer
Expand All @@ -45,19 +45,19 @@ class ConnectionInfo {
* @return std::string the serial number field of the peer certificate. Returns "" if
* there is no peer certificate, or no serial number.
**/
virtual std::string serialNumberPeerCertificate() const PURE;
virtual const std::string& serialNumberPeerCertificate() const PURE;

/**
* @return std::string the issuer field of the peer certificate in RFC 2253 format. Returns "" if
* there is no peer certificate, or no issuer.
**/
virtual std::string issuerPeerCertificate() const PURE;
virtual const std::string& issuerPeerCertificate() const PURE;

/**
* @return std::string the subject field of the peer certificate in RFC 2253 format. Returns "" if
* there is no peer certificate, or no subject.
**/
virtual std::string subjectPeerCertificate() const PURE;
virtual const std::string& subjectPeerCertificate() const PURE;

/**
* @return std::string the URIs in the SAN field of the peer certificate. Returns {} if there is
Expand Down Expand Up @@ -105,7 +105,7 @@ class ConnectionInfo {
/**
* @return std::string the hex-encoded TLS session ID as defined in rfc5246.
**/
virtual std::string sessionId() const PURE;
virtual const std::string& sessionId() const PURE;

/**
* @return uint16_t the standard ID for the ciphers used in the established TLS connection.
Expand All @@ -123,7 +123,7 @@ class ConnectionInfo {
* @return std::string the TLS version (e.g., TLSv1.2, TLSv1.3) used in the established TLS
* connection.
**/
virtual std::string tlsVersion() const PURE;
virtual const std::string& tlsVersion() const PURE;
};

using ConnectionInfoConstSharedPtr = std::shared_ptr<const ConnectionInfo>;
Expand Down
18 changes: 12 additions & 6 deletions 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 @@ -475,11 +475,17 @@ std::string SslSocketInfo::ciphersuiteString() const {
return SSL_CIPHER_get_name(cipher);
}

std::string SslSocketInfo::tlsVersion() const { return SSL_get_version(ssl_.get()); }
const std::string& SslSocketInfo::tlsVersion() const {
if (!cached_tls_version_.empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that we are passing this around via shared_ptr, I'm mildly concerned about potential thread safety with the cached strings. Should we be using read/write locking for the mutable variables? @lizan WDYT? (I can't think of a situation today in which this would break but I'm not sure about the future.)

Copy link
Member

Choose a reason for hiding this comment

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

We don't pass this shared ptr across threads, I think this is fine for now. Do we plan to have multiple thread to process same connection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a side note, SSL object from boringssl is not thread-safe.

Copy link
Member

Choose a reason for hiding this comment

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

No there is no plan to pass connections. OK SGTM.

return cached_tls_version_;
}
cached_tls_version_ = SSL_get_version(ssl_.get());
return cached_tls_version_;
}

absl::string_view SslSocket::failureReason() const { return failure_reason_; }

std::string SslSocketInfo::serialNumberPeerCertificate() const {
const std::string& SslSocketInfo::serialNumberPeerCertificate() const {
if (!cached_serial_number_peer_certificate_.empty()) {
return cached_serial_number_peer_certificate_;
}
Expand All @@ -492,7 +498,7 @@ std::string SslSocketInfo::serialNumberPeerCertificate() const {
return cached_serial_number_peer_certificate_;
}

std::string SslSocketInfo::issuerPeerCertificate() const {
const std::string& SslSocketInfo::issuerPeerCertificate() const {
if (!cached_issuer_peer_certificate_.empty()) {
return cached_issuer_peer_certificate_;
}
Expand All @@ -505,7 +511,7 @@ std::string SslSocketInfo::issuerPeerCertificate() const {
return cached_issuer_peer_certificate_;
}

std::string SslSocketInfo::subjectPeerCertificate() const {
const std::string& SslSocketInfo::subjectPeerCertificate() const {
if (!cached_subject_peer_certificate_.empty()) {
return cached_subject_peer_certificate_;
}
Expand All @@ -518,7 +524,7 @@ std::string SslSocketInfo::subjectPeerCertificate() const {
return cached_subject_peer_certificate_;
}

std::string SslSocketInfo::subjectLocalCertificate() const {
const std::string& SslSocketInfo::subjectLocalCertificate() const {
if (!cached_subject_local_certificate_.empty()) {
return cached_subject_local_certificate_;
}
Expand Down Expand Up @@ -547,7 +553,7 @@ absl::optional<SystemTime> SslSocketInfo::expirationPeerCertificate() const {
return Utility::getExpirationTime(*cert);
}

std::string SslSocketInfo::sessionId() const {
const std::string& SslSocketInfo::sessionId() const {
if (!cached_session_id_.empty()) {
return cached_session_id_;
}
Expand Down
13 changes: 7 additions & 6 deletions 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 @@ -49,21 +49,21 @@ class SslSocketInfo : public Envoy::Ssl::ConnectionInfo {
bool peerCertificatePresented() const override;
std::vector<std::string> uriSanLocalCertificate() const override;
const std::string& sha256PeerCertificateDigest() const override;
std::string serialNumberPeerCertificate() const override;
std::string issuerPeerCertificate() const override;
std::string subjectPeerCertificate() const override;
std::string subjectLocalCertificate() const override;
const std::string& serialNumberPeerCertificate() const override;
const std::string& issuerPeerCertificate() const override;
const std::string& subjectPeerCertificate() const override;
const std::string& subjectLocalCertificate() const override;
std::vector<std::string> uriSanPeerCertificate() const override;
const std::string& urlEncodedPemEncodedPeerCertificate() const override;
const std::string& urlEncodedPemEncodedPeerCertificateChain() const override;
std::vector<std::string> dnsSansPeerCertificate() const override;
std::vector<std::string> dnsSansLocalCertificate() const override;
absl::optional<SystemTime> validFromPeerCertificate() const override;
absl::optional<SystemTime> expirationPeerCertificate() const override;
std::string sessionId() const override;
const std::string& sessionId() const override;
uint16_t ciphersuiteId() const override;
std::string ciphersuiteString() const override;
std::string tlsVersion() const override;
const std::string& tlsVersion() const override;

SSL* rawSslForTest() const { return ssl_.get(); }

Expand All @@ -82,6 +82,7 @@ class SslSocketInfo : public Envoy::Ssl::ConnectionInfo {
mutable std::vector<std::string> cached_dns_san_peer_certificate_;
mutable std::vector<std::string> cached_dns_san_local_certificate_;
mutable std::string cached_session_id_;
mutable std::string cached_tls_version_;
};

class SslSocket : public Network::TransportSocket,
Expand Down
44 changes: 26 additions & 18 deletions test/common/access_log/access_log_formatter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,14 +279,17 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("DOWNSTREAM_LOCAL_SUBJECT");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, subjectLocalCertificate()).WillRepeatedly(Return("subject"));
const std::string subject_local = "subject";
EXPECT_CALL(*connection_info, subjectLocalCertificate())
.WillRepeatedly(ReturnRef(subject_local));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("subject", upstream_format.format(header, header, header, stream_info));
}
{
StreamInfoFormatter upstream_format("DOWNSTREAM_LOCAL_SUBJECT");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, subjectLocalCertificate()).WillRepeatedly(Return(""));
EXPECT_CALL(*connection_info, subjectLocalCertificate())
.WillRepeatedly(ReturnRef(EMPTY_STRING));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("-", upstream_format.format(header, header, header, stream_info));
}
Expand All @@ -298,14 +301,15 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_SUBJECT");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, subjectPeerCertificate()).WillRepeatedly(Return("subject"));
const std::string subject_peer = "subject";
EXPECT_CALL(*connection_info, subjectPeerCertificate()).WillRepeatedly(ReturnRef(subject_peer));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("subject", upstream_format.format(header, header, header, stream_info));
}
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_SUBJECT");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, subjectPeerCertificate()).WillRepeatedly(Return(""));
EXPECT_CALL(*connection_info, subjectPeerCertificate()).WillRepeatedly(ReturnRef(EMPTY_STRING));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("-", upstream_format.format(header, header, header, stream_info));
}
Expand All @@ -317,14 +321,15 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("DOWNSTREAM_TLS_SESSION_ID");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, sessionId()).WillRepeatedly(Return("deadbeef"));
const std::string session_id = "deadbeef";
EXPECT_CALL(*connection_info, sessionId()).WillRepeatedly(ReturnRef(session_id));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("deadbeef", upstream_format.format(header, header, header, stream_info));
}
{
StreamInfoFormatter upstream_format("DOWNSTREAM_TLS_SESSION_ID");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, sessionId()).WillRepeatedly(Return(""));
EXPECT_CALL(*connection_info, sessionId()).WillRepeatedly(ReturnRef(EMPTY_STRING));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("-", upstream_format.format(header, header, header, stream_info));
}
Expand Down Expand Up @@ -357,14 +362,15 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("DOWNSTREAM_TLS_VERSION");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, tlsVersion()).WillRepeatedly(Return("TLSv1.2"));
std::string tlsVersion = "TLSv1.2";
EXPECT_CALL(*connection_info, tlsVersion()).WillRepeatedly(ReturnRef(tlsVersion));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("TLSv1.2", upstream_format.format(header, header, header, stream_info));
}
{
StreamInfoFormatter upstream_format("DOWNSTREAM_TLS_VERSION");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, tlsVersion()).WillRepeatedly(Return(""));
EXPECT_CALL(*connection_info, tlsVersion()).WillRepeatedly(ReturnRef(EMPTY_STRING));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("-", upstream_format.format(header, header, header, stream_info));
}
Expand Down Expand Up @@ -399,15 +405,17 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_SERIAL");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
const std::string serial_number = "b8b5ecc898f2124a";
EXPECT_CALL(*connection_info, serialNumberPeerCertificate())
.WillRepeatedly(Return("b8b5ecc898f2124a"));
.WillRepeatedly(ReturnRef(serial_number));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("b8b5ecc898f2124a", upstream_format.format(header, header, header, stream_info));
}
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_SERIAL");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, serialNumberPeerCertificate()).WillRepeatedly(Return(""));
EXPECT_CALL(*connection_info, serialNumberPeerCertificate())
.WillRepeatedly(ReturnRef(EMPTY_STRING));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("-", upstream_format.format(header, header, header, stream_info));
}
Expand All @@ -419,17 +427,17 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_ISSUER");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, issuerPeerCertificate())
.WillRepeatedly(
Return("CN=Test CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US"));
const std::string issuer_peer =
"CN=Test CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US";
EXPECT_CALL(*connection_info, issuerPeerCertificate()).WillRepeatedly(ReturnRef(issuer_peer));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("CN=Test CA,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US",
upstream_format.format(header, header, header, stream_info));
}
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_ISSUER");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, issuerPeerCertificate()).WillRepeatedly(Return(""));
EXPECT_CALL(*connection_info, issuerPeerCertificate()).WillRepeatedly(ReturnRef(EMPTY_STRING));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("-", upstream_format.format(header, header, header, stream_info));
}
Expand All @@ -441,17 +449,17 @@ TEST(AccessLogFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_SUBJECT");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, subjectPeerCertificate())
.WillRepeatedly(
Return("CN=Test Server,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US"));
const std::string subject_peer =
"CN=Test Server,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US";
EXPECT_CALL(*connection_info, subjectPeerCertificate()).WillRepeatedly(ReturnRef(subject_peer));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("CN=Test Server,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US",
upstream_format.format(header, header, header, stream_info));
}
{
StreamInfoFormatter upstream_format("DOWNSTREAM_PEER_SUBJECT");
auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
EXPECT_CALL(*connection_info, subjectPeerCertificate()).WillRepeatedly(Return(""));
EXPECT_CALL(*connection_info, subjectPeerCertificate()).WillRepeatedly(ReturnRef(EMPTY_STRING));
EXPECT_CALL(stream_info, downstreamSslConnection()).WillRepeatedly(Return(connection_info));
EXPECT_EQ("-", upstream_format.format(header, header, header, stream_info));
}
Expand Down
5 changes: 3 additions & 2 deletions test/common/http/codec_client_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ using testing::NiceMock;
using testing::Pointee;
using testing::Ref;
using testing::Return;
using testing::ReturnRef;
using testing::Throw;

namespace Envoy {
Expand Down Expand Up @@ -260,9 +261,9 @@ TEST_F(CodecClientTest, WatermarkPassthrough) {
}

TEST_F(CodecClientTest, SSLConnectionInfo) {
const auto session_id = "D62A523A65695219D46FE1FFE285A4C371425ACE421B110B5B8D11D3EB4D5F0B";
std::string session_id = "D62A523A65695219D46FE1FFE285A4C371425ACE421B110B5B8D11D3EB4D5F0B";
auto connection_info = std::make_shared<NiceMock<Ssl::MockConnectionInfo>>();
ON_CALL(*connection_info, sessionId()).WillByDefault(Return(session_id));
ON_CALL(*connection_info, sessionId()).WillByDefault(ReturnRef(session_id));
EXPECT_CALL(*connection_, ssl()).WillRepeatedly(Return(connection_info));
connection_cb_->onEvent(Network::ConnectionEvent::Connected);
EXPECT_NE(nullptr, stream_info_.downstreamSslConnection());
Expand Down
8 changes: 4 additions & 4 deletions test/common/http/conn_manager_utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -932,8 +932,8 @@ TEST_F(ConnectionManagerUtilityTest, MtlsSanitizeSetClientCert) {
EXPECT_CALL(*ssl, uriSanLocalCertificate()).WillOnce(Return(local_uri_sans));
std::string expected_sha("abcdefg");
EXPECT_CALL(*ssl, sha256PeerCertificateDigest()).WillOnce(ReturnRef(expected_sha));
EXPECT_CALL(*ssl, subjectPeerCertificate())
.WillOnce(Return("/C=US/ST=CA/L=San Francisco/OU=Lyft/CN=test.lyft.com"));
std::string peer_subject("/C=US/ST=CA/L=San Francisco/OU=Lyft/CN=test.lyft.com");
EXPECT_CALL(*ssl, subjectPeerCertificate()).WillOnce(ReturnRef(peer_subject));
const std::vector<std::string> peer_uri_sans{"test://foo.com/fe"};
EXPECT_CALL(*ssl, uriSanPeerCertificate()).WillRepeatedly(Return(peer_uri_sans));
std::string expected_pem("abcde=");
Expand Down Expand Up @@ -973,8 +973,8 @@ TEST_F(ConnectionManagerUtilityTest, MtlsSanitizeSetClientCertPeerSanEmpty) {
EXPECT_CALL(*ssl, uriSanLocalCertificate()).WillOnce(Return(local_uri_sans));
std::string expected_sha("abcdefg");
EXPECT_CALL(*ssl, sha256PeerCertificateDigest()).WillOnce(ReturnRef(expected_sha));
EXPECT_CALL(*ssl, subjectPeerCertificate())
.WillOnce(Return("/C=US/ST=CA/L=San Francisco/OU=Lyft/CN=test.lyft.com"));
std::string peer_subject = "/C=US/ST=CA/L=San Francisco/OU=Lyft/CN=test.lyft.com";
EXPECT_CALL(*ssl, subjectPeerCertificate()).WillOnce(ReturnRef(peer_subject));
EXPECT_CALL(*ssl, uriSanPeerCertificate()).WillRepeatedly(Return(std::vector<std::string>()));
ON_CALL(connection_, ssl()).WillByDefault(Return(ssl));
ON_CALL(config_, forwardClientCert())
Expand Down
Loading