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.

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

Fixed ForwardClientCertFilter to handle SAN types other than DNS and URI #1850

Merged
merged 1 commit into from Mar 14, 2018

Conversation

Projects
None yet
5 participants
@shakti-das
Copy link
Contributor

commented Mar 3, 2018

Signed-off-by: Shakti Das shakti.das2002@gmail.com

Problem
The ForwardClientCertFilter will get a MatchError if it encounters SAN types other than DNS and URI.

Solution
Added a default case in ForwardClientCertFilter.

Validation
The TLS related tests were run to verify it causes no regression. The tests were modified to verify ForwardClientCertFilter works with additional IP entries in SAN.

Fixes #1849

Fixed ForwardClientCertFilter to handle SAN types other than DNS and URI
Signed-off-by: Shakti Das shakti.das2002@gmail.com
@shakti-das

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

@briansmith @wmorgan @adleong this is a follow-up PR to my original one #1826

It fixes the default case for SAN entries other than URI and DNS in ForwardClientCert. Modified the test config as well to add an IP SAN in order to catch this and similar errors in future. Please review. Thanks.

@briansmith
Copy link

left a comment

LGTM. I shouldn't be the one to approve this because I'm not familiar with Scala and such familiarity is needed to properly review this.

@adleong

adleong approved these changes Mar 4, 2018

Copy link
Member

left a comment

Nice, good catch. ⭐️

@shakti-das

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

@wmorgan @adleong @briansmith Hi, I was wondering if this would be merged before releasing 1.3.7 as we are waiting on this fix for our internal implementation.

@hawkw hawkw requested a review from olix0r Mar 14, 2018

@hawkw

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@shakti-das We're not planning on cutting 1.3.7 until next week, and we'd like to get this PR merged today pending @olix0r's sign-off. :)

@olix0r

olix0r approved these changes Mar 14, 2018

@hawkw hawkw merged commit 66fa4d2 into linkerd:master Mar 14, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@hawkw

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

And it's merged! Thanks @shakti-das!

@shakti-das shakti-das deleted the shakti-das:shakti/fix-xfcc-default branch Mar 24, 2018

@adleong adleong referenced this pull request Apr 5, 2018

Merged

1.3.7 #1888

adleong added a commit that referenced this pull request Apr 5, 2018

1.3.7 (#1888)
Linkerd 1.3.7 includes memory leak fixes, tons of improvements for Consul, and more!  This release features contributions from ThreeComma, [NCBI](https://github.com/ncbi), WePay, [Salesforce](https://github.com/salesforce), [Homeaway](https://github.com/homeaway), Prosoft, and [Buoyant](https://github.com/buoyantio).

* Add support for more types of client certificates in the ForwardClientCertFilter ([#1850](#1850)).
* Improve documentation on how to override the base Docker image ([#1867](#1867)).
* Improve the efficientcy of the dtab delegator UI ([#1862](#1862)).
* Add a command line flag for config file validation ([#1854](#1854)).
* Fix a bug where the wrong timezone was being used in access logs ([#1851](#1851)).
* Add the ability to explicitly disable TLS for specific clients ([#1856](#1856)).
* Consul:
  * Add support for TLS-encrypted communication with Consul ([#1842](#1842)).
  * Fix a connection leak to Consul ([#1877](#1877)).
  * Fix a bug where Linkerd would timeout requests to non-existent Consul datacenters ([#1863](#1877)).
* Remove many alarming but harmless error messages from Linkerd and Namerd logs ([#1871](#1871), [#1884](#1884), [#1875](#1875)).
* Fix ByteBuffer memory leaks in HTTP/2 ([#1879](#1879), [#1858](#1858)).

Signed-off-by: Alex Leong <alex@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.