Skip to content

Commit

Permalink
make getters const-ref (envoyproxy#8192)
Browse files Browse the repository at this point in the history 
Description:
Follow-up to envoyproxy#7911 to make cached values be exposed as const-references, saving on a copy of a string during retrieval.

Risk Level: low
Testing: updated mocks to return references
Docs Changes: none
Release Notes: none

Signed-off-by: Kuat Yessenov <kuat@google.com>
  • Loading branch information
@kyessenov
kyessenov committed Sep 10, 2019
1 parent d58c085 commit 384be02
Show file tree
Hide file tree
Showing 14 changed files with 149 additions and 83 deletions.
12 changes: 6 additions & 6 deletions include/envoy/ssl/connection.h
View file
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
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()) {
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
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
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
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
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

0 comments on commit 384be02

Please sign in to comment.