Skip to content

tls: Add subjectLocalCertificate to get local cert subject.#2319

Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
colabsaumoh:ssl-local-subject
Jan 9, 2018
Merged

tls: Add subjectLocalCertificate to get local cert subject.#2319
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
colabsaumoh:ssl-local-subject

Conversation

@saumoh
Copy link
Copy Markdown
Contributor

@saumoh saumoh commented Jan 5, 2018

Description:
Add subjectLocalCertificate().
In filling out the external auth proto's Principal It may be required that the local certificates subject needs to be used.
Here I am adding the missing api that in tls/ssl so that the value may be extracted from the certificate.

See also #2291

Risk Level: Low

Testing:
Wrote ut that test the subjects in ssl (local and peer). Also added test to check for local SAN.

Docs Changes:
N/A
Release Notes:
N/A

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. @ggreenway or @PiotrSikora any comments?

@mattklein123
Copy link
Copy Markdown
Member

@saumoh please merge master and fix tests also.

Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM, with one possible comment/doc improvement.

virtual std::string uriSanLocalCertificate() PURE;

/**
* @return the subject field of the local certificate in RFC 2253 format. Returns "" if there is
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 on citing the RFC in the comment. Can you please also put a simple example value, so I don't have to look up the RFC to know approximately what this function returns?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@saumoh Can you either make the doc change here or comment on why it's not a good idea?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ggreenway I should have mentioned that i added some sample values in one of the follow up commits.
409f159#diff-ded6386c66f63a1375432303053ca7eeR290

Saurabh Mohan added 3 commits January 8, 2018 09:58
Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>
Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>
Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>
@saumoh
Copy link
Copy Markdown
Contributor Author

saumoh commented Jan 8, 2018

@ggreenway @mattklein123 Thx for the review. i've addressed review feedback and tests are passing now :-)

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

nice, thanks!

@mattklein123 mattklein123 merged commit 2d8665a into envoyproxy:master Jan 9, 2018
@saumoh saumoh deleted the ssl-local-subject branch May 4, 2018 17:28
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* working except -id

* add logs

* review comments
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: Updates the exposed KeyValueStore type to be a more traditional implementable interface, and provides a simple persisting implementation based on Android SharedPreferences.
Risk: Low
Testing: Application

Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: Updates the exposed KeyValueStore type to be a more traditional implementable interface, and provides a simple persisting implementation based on Android SharedPreferences.
Risk: Low
Testing: Application

Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
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.

3 participants