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

pkg/transport: Add test for "deny incoming peer certs with wrong IP SAN" #10729

Closed

Conversation

wenjiaswe
Copy link
Contributor

Thanks to @MartinWeindel who actually wrote the test #10524. The PR #10524 is to add a flag so IP checking for incoming peer certs can be skipped if wanted, and @MartinWeindel found out that there was no test added at first place for the implementation of #7687. So all I did here is just to add the test to 3.2 when this feature was introduced.

@wenjiaswe
Copy link
Contributor Author

wenjiaswe commented May 15, 2019

When I run the test, it turned out that bad IP address was actually accepted, it seems like a bug.

$ PASSES=unit PKG=./pkg/transport TESTCASE="\bTestNewListenerTLSInfoClientVerify\b"   ./test -v
Starting 'unit' pass at Wed May 15 15:39:29 PDT 2019
Running unit tests...
=== RUN   TestNewListenerTLSInfoClientVerify
--- FAIL: TestNewListenerTLSInfoClientVerify (0.87s)
	listener_test.go:173: accepted for bad client address: goodClientHost=false
FAIL
coverage: 34.9% of statements
FAIL	github.com/coreos/etcd/pkg/transport	0.925s

@wenjiaswe wenjiaswe changed the title transport: Add test for "deny incoming peer certs with wrong IP SAN" [WIP] transport: Add test for "deny incoming peer certs with wrong IP SAN" May 16, 2019
@wenjiaswe
Copy link
Contributor Author

It seems like in the test, peer cert in the test is not successfully created, len(st.PeerCertificates) is still zero in the test. The test works fine at HEAD, but not in v3.2.26, I need to spend some time and find out why.

if len(st.PeerCertificates) > 0 {

@wenjiaswe wenjiaswe changed the title [WIP] transport: Add test for "deny incoming peer certs with wrong IP SAN" pkg/transport: Add test for "deny incoming peer certs with wrong IP SAN" May 22, 2019
@wenjiaswe
Copy link
Contributor Author

Fixed. @jingyih would you please take another look? Thanks!
cc @hexfusion as well.

@jingyih
Copy link
Contributor

jingyih commented Jun 9, 2019

Discussed with @wenjiaswe. We will not backport a test.

@jingyih jingyih closed this Jun 9, 2019
@wenjiaswe wenjiaswe deleted the addTestforPeerIPcheck branch October 15, 2019 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants