Skip to content

Add additional TLS 1.3 cipher suites #208

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.

Sign up for GitHub

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

Jump to bottom
Merged
mbyczkowski merged 2 commits into from
Feb 26, 2020
Merged

Add additional TLS 1.3 cipher suites #208

mbyczkowski merged 2 commits into from
Feb 26, 2020

Conversation

@jason-riddle
Copy link
Contributor

@jason-riddle jason-riddle commented Feb 21, 2020 •
edited
Loading

I thought this was a solution to #202, but instead it attempts to solve the following issue when connect to a TLS 1.3 endpoint.

Notice that Cipher Suite has the value UNKNOWN_1301

certigo connect --verbose squareup.com:443
** TLS Connection **
Version: TLS 1.3
Cipher Suite: UNKNOWN_1301

However, after applying this fix the Cipher Suite section is empty

./certigo-with-tls13-ciphers connect --verbose squareup.com:443
** TLS Connection **
Version: TLS 1.3
Cipher Suite:

1301 maps to TLS_AES_128_GCM_SHA256 which I've already included in this PR

https://github.com/golang/go/blob/go1.13.1/src/crypto/tls/cipher_suites.go#L465

TLS_AES_128_GCM_SHA256       uint16 = 0x1301

I don't have time to look into this, so I'm marking this as WIP unless someone can offer a suggestion.

@claassistantio
Copy link

claassistantio commented Feb 21, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@royxu6
Copy link
Contributor

royxu6 commented Feb 24, 2020 •
edited
Loading

This is because the new cipher suites do not contain "_WITH_" in the Slug name. The new cipher suites do not explicitly specify the key exchange algorithm.

func explainCipher(d description) description {
    kexAndCipher := strings.Split(d.Slug, "_WITH_")
    if len(kexAndCipher) == 2 {
        d.Name = fmt.Sprintf("%s key exchange, %s cipher", kexAndCipher[0][len("TLS_"):], kexAndCipher[1])
    } else {
	d.Name = fmt.Sprintf("%s cipher", d.Slug[len("TLS_"):])
    }
    return d
}

Above is a change for the explainCipher function that takes this into account

@jason-riddle
Copy link
Contributor Author

jason-riddle commented Feb 25, 2020

Thanks @captiosus, I'll update my PR and verify the change.

Check length when splitting on "_WITH_" and handle if not using TLS 1.3.
7616e1f 
The new TLS 1.3 cipher suites do not specify the key exchange algorithm
and as a result "_WITH_" is no longer present.
@jason-riddle
Copy link
Contributor Author

jason-riddle commented Feb 25, 2020

That change did work. new output looks like this for tls 1.3:

./certigo-with-tls13-ciphers connect --verbose squareup.com:443 | head -3
** TLS Connection **
Version: TLS 1.3
Cipher Suite: AES_128_GCM_SHA256 cipher

and tls <1.3 appears to be unchanged

./certigo-with-tls13-ciphers connect --verbose reddit.com:443 | head -3
** TLS Connection **
Version: TLS 1.2
Cipher Suite: ECDHE_RSA key exchange, AES_128_GCM_SHA256 cipher

@jason-riddle
Copy link
Contributor Author

jason-riddle commented Feb 25, 2020

Removing WIP.

@jason-riddle jason-riddle changed the title [WIP] Add additional TLS 1.3 cipher suites Add additional TLS 1.3 cipher suites Feb 25, 2020
mbyczkowski
mbyczkowski approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@mbyczkowski mbyczkowski left a comment

Choose a reason for hiding this comment

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

This change looks good to me. Thanks for the PR! 🙂

@mbyczkowski mbyczkowski merged commit 5cba65c into square:master Feb 26, 2020
@jason-riddle jason-riddle deleted the tls13-ciphers branch February 26, 2020 19:55
@mbyczkowski mbyczkowski mentioned this pull request May 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbyczkowski mbyczkowski mbyczkowski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jason-riddle @claassistantio @royxu6 @mbyczkowski