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

Got it to failing, but... #656

Closed
wants to merge 7 commits into from
Closed

Got it to failing, but... #656

wants to merge 7 commits into from

Conversation

ineiti
Copy link
Member

@ineiti ineiti commented Aug 20, 2020

The update to the new TLS handshake is faulty. The test in #655 only tests old->old and new->new, but not new->old and old->new (which fails).

This PR tests all combinations, but fails. Currently I don't know why :(

The relevant test is here: https://github.com/dedis/onet/pull/656/files#diff-f183c5e9370db10aaa3413143732a5abR46

@ineiti ineiti added this to WIP in Cothority via automation Aug 20, 2020
@ineiti ineiti requested a review from jeffallen August 20, 2020 11:33
if !found {
return xerrors.Errorf("No onet-pubkey URIs match the expected public key %v", pubToCN(them.Public))
if noURIs {
err = cert.VerifyHostname(pubToCN(them.Public))
Copy link
Contributor

@jeffallen jeffallen Aug 20, 2020

Choose a reason for hiding this comment

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

This call will not pass, no matter what, if you are testing with Go 1.15. The idea of this PR is to provide two implementations, one like v3.2.4 and one like v3.2.5 and mix and match them. That's cool, but then you can only get it to pass with Go 1.14.

I installed Go 1.14 and then ran the unit test like this:

brew install go@1.14
/usr/local/Cellar/go@1.14/1.14.7/bin/go test -run TestTLS -v

They pass, with my simpler version in bf6194e.

@@ -184,6 +185,7 @@ func (cm *certMaker) get(nonce []byte) (*tls.Certificate, error) {
// test code to get ahold of and modify.
if testNoURIs {
tmpl.URIs = nil
tmpl.MaxPathLen = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This is necessary as a true repro of v3.2.4 behaviour, but it is not important. It will cause Go 1.15's server-side TLS to fail with an "internal error".

network/tcp.go Outdated
@@ -116,7 +116,7 @@ func NewTCPConn(addr Address, suite Suite) (conn *TCPConn, err error) {
func (c *TCPConn) Receive() (env *Envelope, e error) {
buff, err := c.receiveRaw()
if err != nil {
return nil, xerrors.Errorf("receiving: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why, but this change causes TestRouterErrorHandling to fail.

@jeffallen
Copy link
Contributor

So I'm a little unclear on where the changes to the error logging came from, don't know if they need to be there, in which case we need to fix the tests.

Also, not clear on what this tells us: it seems like this proves that this change is backwards compatible, but clearly the experience with the production network says it was not.

@ineiti
Copy link
Member Author

ineiti commented Aug 21, 2020

Also, not clear on what this tells us: it seems like this proves that this change is backwards compatible, but clearly the experience with the production network says it was not.

What has been 'tested in prod' and failed is the following:
old (1.14) -> new (1.15)
And this is very hard to test - should be more of an integration test with a test.sh that takes old code and compiles it with 1.14 and tries to connect to new code compiled with 1.15...

Closing in favour of #657

@ineiti ineiti closed this Aug 21, 2020
Cothority automation moved this from WIP to Closed Aug 21, 2020
@ineiti ineiti deleted the check_new_tls_handshake branch August 21, 2020 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Cothority
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants