admin: add support for displaying san in certs end point by ramaraochavali · Pull Request #4566 · envoyproxy/envoy

ramaraochavali · 2018-09-29T08:12:48Z

Signed-off-by: Rama rama.rao@salesforce.com
Description: Adds support for displaying Subject Alternate Names as a comma separated list of string in /certs admin end point
Risk Level: Low
Testing: Automated
Docs Changes: Updated
Release Notes: Updated

Signed-off-by: Rama <rama.rao@salesforce.com>

ggreenway

This seems reasonable, but I'm curious how this output is being used, and if anyone depends on the exact formatting of it. Anyone at lyft want to chime in? @mattklein123 @ccaraman @junr03 @danielhochman ?

Also, the output is formatted pretty strangely: It outputs repeated json objects (but without a json array [] outside), and the values contain free-form text of "key: value, key2: value2," format. Should we fix this to be proper json, with each key:value as a json key:value? Then adding new fields in the future shouldn't break anyone.

ggreenway · 2018-10-02T16:06:47Z

 namespace Ssl {

-std::string Utility::getSerialNumberFromCertificate(X509& cert) {
+const std::string Utility::getSerialNumberFromCertificate(X509& cert) {


revert: there's no reason to make something returned by-value const

ggreenway · 2018-10-02T16:14:39Z

  return "";
 }

+const std::vector<std::string> Utility::getSubjectAltNames(X509& cert) {


I don't think this function is needed. There is already getDnsSansFromCertificate() and getUriSanFromCertificate(). You can just combine the results from those two.

Are you referring to these functions?

envoy/source/common/ssl/ssl_socket.h

Line 72 in f936fc6

// TODO: Move helper functions to the `Ssl::Utility` namespace.

There is a TODO to move them to Utility. So I have added them in Utitlity now and will do a follow-up PR to use these functions there as well. WDYT? If you are referring to some thing else, please let me know. I will use them.

mattklein123 · 2018-10-02T17:18:50Z

This seems reasonable, but I'm curious how this output is being used, and if anyone depends on the exact formatting of it.

Right now this data is being just used for debugging. IMO we should take this opportunity to shift to a proto as we have done elsewhere. We can put this in the v1alpha namespace and then output JSON? Thoughts?

ramaraochavali · 2018-10-03T04:16:04Z

I agree that this format is not scalable. I was thinking that moving to proto/json might require a broader discussion. But if every one thinks, moving to proto/json is right thing to do here, I can turn in this PR in to that.

mattklein123 · 2018-10-03T04:37:48Z

@ramaraochavali yeah I would just switch it. It's pretty easy to do.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-03T10:09:35Z

@mattklein123 ok. Moved to protobuf/json implementation. Here is how it looks now. PTAL.

{
 "certificates": [
  {
   "ca_cert": {
    "path": "/git/envoy_files/certs/client-certs/ca.pem",
    "serial_number": "4114ea7c5d6cc0779e57c2b97c12a1940f2f9b7a",
    "subject_alt_names": [],
    "days_until_expiration": "2768"
   },
   "cert_chain": {
    "path": "/git/envoy_files/certs/client-certs/ca.pem",
    "serial_number": "5966c392",
    "subject_alt_names": [
     {
      "type": "DNS",
      "name": "localhost"
     }
    ],
    "days_until_expiration": "905"
   }
  },
  {
   "ca_cert": {
    "path": "/git/envoy_files/certs/client-certs/ca.pem",
    "serial_number": "4114ea7c5d6cc0779e57c2b97c12a1940f2f9b7a",
    "subject_alt_names": [],
    "days_until_expiration": "2768"
   },
   "cert_chain": {
    "path": "/git/envoy_files/certs/client-certs/ca.pem",
    "serial_number": "5966c392",
    "subject_alt_names": [
     {
      "type": "DNS",
      "name": "localhost"
     }
    ],
    "days_until_expiration": "905"
   }
  }
 ]
}

Signed-off-by: Rama <rama.rao@salesforce.com>

mattklein123 · 2018-10-03T16:08:45Z

LGTM on the format! We can always change it later since it's alpha. I will review today. Thank you!

mattklein123

Very nice. 1 high level comment.

mattklein123 · 2018-10-03T17:22:02Z

  /**
-   * @return a string of ca certificate path, certificate serial number and days until certificate
-   * expiration
+   * @return a JSON string representation of certificate conforming to proto admin.v2alpha.certs.


Would it be better to have these functions return the proto objects? Then the admin code can actually merge the protos and do the JSON conversion? I think that would be more useful in the future for programmatic access of this information?

Ok. Will make that change

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-04T08:00:27Z

@mattklein123 changed the methods to return proto objects. PTAL.

mattklein123

LGTM, small question/comment. @ggreenway can you take another pass and see if this works for you?

mattklein123 · 2018-10-04T15:53:21Z

+CertificateDetailsPtr ContextImpl::getCaCertInformation() const {
  if (ca_cert_ == nullptr) {
-    return "";
+    return std::make_unique<envoy::admin::v2alpha::CertificateDetails>();


Shouldn't this return nullptr to indicate no details?

mattklein123 · 2018-10-04T15:53:30Z

+CertificateDetailsPtr ContextImpl::getCertChainInformation() const {
  if (cert_chain_ == nullptr) {
-    return "";
+    return std::make_unique<envoy::admin::v2alpha::CertificateDetails>();


Same comment?

ggreenway · 2018-10-04T15:47:42Z

+CertificateDetailsPtr ContextImpl::getCaCertInformation() const {
  if (ca_cert_ == nullptr) {
-    return "";
+    return std::make_unique<envoy::admin::v2alpha::CertificateDetails>();


return nullptr in all these error cases

ggreenway · 2018-10-04T15:52:37Z

-
    envoy::admin::v2alpha::CertificateDetails* ca_certificate = certificate.mutable_ca_cert();
-    MessageUtil::loadFromJson(context.getCaCertInformation(), *ca_certificate);
+    ca_certificate->MergeFrom(*context.getCaCertInformation());


I think you can just assign, instead of doing MergeFrom, given that ca_certificate should be empty (eg we can replace instead of merge).

ggreenway · 2018-10-04T15:52:43Z

+    ca_certificate->MergeFrom(*context.getCaCertInformation());
    envoy::admin::v2alpha::CertificateDetails* cert_chain = certificate.mutable_cert_chain();
-    MessageUtil::loadFromJson(context.getCertChainInformation(), *cert_chain);
+    cert_chain->MergeFrom(*context.getCertChainInformation());


Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-05T04:16:26Z

@mattklein123 @ggreenway addressed the feedback. PTAL.

ggreenway

lgtm

ggreenway · 2018-10-05T16:21:21Z

Merge conflict :(

mattklein123 · 2018-10-05T16:27:56Z

    envoy::admin::v2alpha::Certificate& certificate = *certificates.add_certificates();
    envoy::admin::v2alpha::CertificateDetails* ca_certificate = certificate.mutable_ca_cert();
-    ca_certificate->MergeFrom(*context.getCaCertInformation());
+    *ca_certificate = *context.getCaCertInformation();


Doesn't this need to check for nullptr now? Same below? Can you add a test that covers this?

I thought context will not be created if files does not exist or not valid. Any way it is good to handle that case. I will add a test.

Signed-off-by: Rama <rama.rao@salesforce.com>

lizan · 2018-10-06T06:20:08Z

+}
+
+message SubjectAlternateName {
+  enum Type {


Prefer oneof here per https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md

If I use oneof it would be converted to JSON as if I set empty value.

"subject_alt_names": [ { "name": "localhost", "DNS": "" } ], "days_until_expiration": "903" }

I think what we have as below is more readable. Are you suggesting some thing different here?

"subject_alt_names": [ { "name": "localhost", "type": "DNS" } ], "days_until_expiration": "903" }

No, I meant you should make this

message SubjectAlterativeName { oneof name { string dns = 1; string uri = 2; } }

The JSON would be:

"subject_alt_names": [ { "dns": "example.com" } ]

Ah.. I see. I will make that change

lizan · 2018-10-06T06:20:46Z

+  repeated SubjectAlternateName subject_alt_names = 3;
+
+  // Days until expiration of the Certificate.
+  uint64 days_until_expiration = 4;


Should this just be expiration timestamp?

days_until_expiration looks at the chain also and computes the minimum of cert and chain. I think I will leave it like that for now as stats and others also use the same. If we can add another field to just dump expiration timestamp later?

reg expiration timestamp, just looked at it. There is no direct api now to get it. We will have to add one function , related tests etc. I will add it once this is merged in a follow-up PR.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-06T10:24:22Z

@mattklein123 added test. PTAL.

mattklein123

Thanks, LGTM. One additional small comment that I missed last time. Sorry about that.

mattklein123 · 2018-10-08T03:28:30Z

+  return subject_alt_names;
+}
+
+std::vector<std::string> Utility::getUriSubjectAltNames(X509& cert) {


Sorry, I just noticed this is basically the same function as the previous one. Can you padd the SAN type as a parameter instead?

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-08T04:32:42Z

@mattklein123 no problem. Fixed. PTAL.

lizan · 2018-10-08T05:17:18Z

  EXPECT_FALSE(ContextImpl::dNSNameMatch("lyft.com", ""));
 }

+TEST_F(SslContextImplTest, TestGetSubjectAlternateNamesWithDNS) {


Move these tests to utility_test?

lizan · 2018-10-08T05:19:32Z

+}
+
+TEST_F(SslContextImplTest, TestGetCertInformationWithSAN) {
+  std::string json = R"EOF(


This is v1 config format to be removed soon, can you use v2 format?

I can use for these two tests. But rest of the tests are still using v1 config in this same file. I prefer to leave these two also in v1 for now and cleanup all of them to v2 in a separate PR if that is Ok.

lizan · 2018-10-08T05:21:48Z

+  std::string ca_cert_partial_output(TestEnvironment::substitute(ca_cert_json));
+  std::string cert_chain_partial_output(TestEnvironment::substitute(cert_chain_json));
+  EXPECT_TRUE(MessageUtil::getJsonStringFromMessage(*context->getCaCertInformation(), true, true)
+                  .find(ca_cert_partial_output) != std::string::npos);


Use MessageDifferencer instead of comparing JSON string.

I tried to use MessageDifferencer and setup is becoming complicated because we need to ignore certain fields like days_until_expired etc as they change. So I left this with string comparison.

You should be able to do that with PARTIAL scope. https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.util.message_differencer#MessageDifferencer.Scope.details

lizan · 2018-10-08T05:25:57Z

+  CertificateDetails ca_cert = 1;
+
+  // Details of Certificate Chain
+  CertificateDetails cert_chain = 2;


Shouldn't these be repeated? I know current /certs only print the 1st certificate in ca_cert and leaf certificate information in cert_chain for each TLS context, but we will need a full cert chain and CA cert?

Ok. My goal was to replicate what we have in /certs with subject alt names added. Do you mind creating issue with what you are describing here so that we can add it once it is merged?

Yeah I know you're just replicate current behavior of /certs, so make this repeated, and it is fine to only have one when you fill it.

ok. Do you mean for both cert chain and ca cert?

lizan · 2018-10-08T05:26:18Z

+  FILE* fp = fopen(
+      TestEnvironment::runfilesPath("test/common/ssl/test_data/san_uri_cert.pem").c_str(), "r");
+  EXPECT_NE(fp, nullptr);
+  X509* cert = PEM_read_X509(fp, nullptr, nullptr, nullptr);


bssl::UniquePtr

lizan · 2018-10-08T05:27:34Z

+}
+
+TEST_F(SslContextImplTest, TestGetSubjectAlternateNamesWithUri) {
+  FILE* fp = fopen(


use TestEnvironment::readFileToStringForTest read into a string then use BIO? See how context_impl reads certificate. Also factor out that function for these 3 tests.

ok. I will refactor to this to a function in utility test using the same approach because rest of the context_test_impl also uses the same to be consistent.

Ah ok, filed #4635 to track them.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-08T10:15:13Z

@lizan changed as suggested. PTAL.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-09T00:29:26Z

@mattklein123 rebased master to resolve the conflicts. Can you PTAL?

mattklein123

LGTM, thank you! @lizan can you do the remainder of the review/merge?

lizan · 2018-10-09T21:54:51Z

@ramaraochavali can you merge master?

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-10-10T00:19:25Z

@lizan merged master. PTAL

…4566) *Description*: Adds support for displaying Subject Alternate Names as a comma separated list of string in `/certs` admin end point *Risk Level*: Low *Testing*: Automated *Docs Changes*: Updated *Release Notes*: Updated Signed-off-by: Rama <rama.rao@salesforce.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

ramaraochavali added 3 commits September 29, 2018 13:21

added support for displaying san

46b5cf5

Signed-off-by: Rama <rama.rao@salesforce.com>

docs and formatting

177c36e

Signed-off-by: Rama <rama.rao@salesforce.com>

Merge branch 'master' into fix/add_san_certs

879dc73

Signed-off-by: Rama <rama.rao@salesforce.com>

junr03 assigned ggreenway Oct 1, 2018

ggreenway requested changes Oct 2, 2018

View reviewed changes

moved to protobuf implementation

8fe22f5

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali added 2 commits October 3, 2018 16:24

fix admin integration test

13828d0

Signed-off-by: Rama <rama.rao@salesforce.com>

docs and minor refactor

4701798

Signed-off-by: Rama <rama.rao@salesforce.com>

mattklein123 self-assigned this Oct 3, 2018

mattklein123 reviewed Oct 3, 2018

View reviewed changes

address review feedback

cec4e27

Signed-off-by: Rama <rama.rao@salesforce.com>

mattklein123 reviewed Oct 4, 2018

View reviewed changes

ggreenway requested changes Oct 4, 2018

View reviewed changes

ramaraochavali added 2 commits October 5, 2018 07:45

Merge branch 'master' into fix/add_san_certs

d43426b

Signed-off-by: Rama <rama.rao@salesforce.com>

address minor review comments

ea2af4c

Signed-off-by: Rama <rama.rao@salesforce.com>

ggreenway previously approved these changes Oct 5, 2018

View reviewed changes

mattklein123 requested changes Oct 5, 2018

View reviewed changes

ramaraochavali added 2 commits October 6, 2018 09:00

Merge branch 'master' into fix/add_san_certs

f8fa7b6

Signed-off-by: Rama <rama.rao@salesforce.com>

handle null cert details

44ac253

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali dismissed ggreenway’s stale review via 44ac253 October 6, 2018 05:27

lizan reviewed Oct 6, 2018

View reviewed changes

move to oneof

ab53890

Signed-off-by: Rama <rama.rao@salesforce.com>

mattklein123 requested changes Oct 8, 2018

View reviewed changes

refactor subject alt names

93fef32

Signed-off-by: Rama <rama.rao@salesforce.com>

lizan suggested changes Oct 8, 2018

View reviewed changes

lizan mentioned this pull request Oct 8, 2018

tls: refactor tls tests #4635

Closed

4 tasks

ramaraochavali added 4 commits October 8, 2018 13:11

move tests to utility_test

fdfb01d

Signed-off-by: Rama <rama.rao@salesforce.com>

move to TestUtils

085c90c

Signed-off-by: Rama <rama.rao@salesforce.com>

minor namespace change

db07934

Signed-off-by: Rama <rama.rao@salesforce.com>

repeated cert fields and use messagedifferencer

8a76c41

Signed-off-by: Rama <rama.rao@salesforce.com>

Merge branch 'master' into fix/add_san_certs

1c4217b

Signed-off-by: Rama <rama.rao@salesforce.com>

mattklein123 previously approved these changes Oct 9, 2018

View reviewed changes

lizan previously approved these changes Oct 9, 2018

View reviewed changes

lizan mentioned this pull request Oct 9, 2018

admin: /certs should dump all certificates #4661

Open

Merge branch 'master' into fix/add_san_certs

5984913

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali dismissed stale reviews from lizan and mattklein123 via 5984913 October 10, 2018 00:17

lizan approved these changes Oct 10, 2018

View reviewed changes

lizan merged commit 7403314 into envoyproxy:master Oct 10, 2018

ramaraochavali deleted the fix/add_san_certs branch October 10, 2018 04:25

ramaraochavali mentioned this pull request Oct 11, 2018

tls: refactor utility functions #4686

Merged

jpeach mentioned this pull request Dec 9, 2019

SubjectAlternateName protobuf message omits IP address names #9269

Closed

Conversation

ramaraochavali commented Sep 29, 2018

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Oct 2, 2018

Uh oh!

ramaraochavali commented Oct 3, 2018

Uh oh!

mattklein123 commented Oct 3, 2018

Uh oh!

ramaraochavali commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Oct 3, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Oct 4, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Oct 5, 2018

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Oct 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali Oct 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Oct 6, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Oct 8, 2018

ramaraochavali commented Oct 3, 2018 •

edited

Loading

ramaraochavali Oct 6, 2018 •

edited

Loading