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

V2 Release! First breaking change certificate chains. #142

Merged
merged 3 commits into from
Nov 23, 2019
Merged

V2 Release! First breaking change certificate chains. #142

merged 3 commits into from
Nov 23, 2019

Conversation

Sean-Der
Copy link
Member

Thank you so much @jkralik for this patch, sorry it took me so long :( but we are going to land this now and get in some other stuff!


Breaking changes:

  • certificate, privateKey was replaced
    by certificate (tls.Certificate)
  • verifyPeerCertificates uses array of bytes for chain
    certificate instead of certificate(*x509.Certificate)

@codecov-io
Copy link

codecov-io commented Nov 11, 2019

Codecov Report

Merging #142 into master will decrease coverage by 0.16%.
The diff coverage is 64.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
- Coverage   73.49%   73.33%   -0.17%     
==========================================
  Files          54       54              
  Lines        3260     3285      +25     
==========================================
+ Hits         2396     2409      +13     
- Misses        602      608       +6     
- Partials      262      268       +6
Impacted Files Coverage Δ
listener.go 57.89% <ø> (ø) ⬆️
state.go 66.66% <ø> (ø) ⬆️
crypto_ccm.go 62.5% <ø> (ø) ⬆️
server_handlers.go 85.39% <100%> (ø) ⬆️
config.go 100% <100%> (ø) ⬆️
client_handlers.go 82.96% <100%> (+0.55%) ⬆️
conn.go 80% <100%> (+0.42%) ⬆️
crypto.go 58.2% <50%> (-6.5%) ⬇️
util.go 87.25% <66.66%> (+1.54%) ⬆️
handshake_message_certificate.go 75% <76.47%> (+4.62%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0649d6e...4da5e8c. Read the comment docs.

config.go Show resolved Hide resolved
crypto.go Outdated Show resolved Hide resolved
handshake_message_certificate.go Outdated Show resolved Hide resolved
@daenney
Copy link
Member

daenney commented Nov 18, 2019

I'm a bit puzzled by something. In a few places I now see this show up:

func xxx(... rawCertificates [][]byte ...) ... {
if len(rawCertificates) == 0 {
		return errLengthMismatch
}
certificate, err := x509.ParseCertificate(rawCertificates[0])

Since it seems we only ever care/check for/validate the first one, what is the idea behind accepting a [][]byte? And if len(rawCertificates) > 1, what happens to those?

@Sean-Der
Copy link
Member Author

@jkralik hey (no rush though)! any thoughts on @daenney question, I was just fixing merge conflicts haven't really dove in yet.

@hjames9 mind giving it another look over? I tried to use constants everywhere/simplify stuff as much as I possible.

crypto.go Outdated Show resolved Hide resolved
@jkralik
Copy link
Contributor

jkralik commented Nov 22, 2019

@Sean-Der Sorry for late response but I was very busy. I created PR #152 for this branch to fix issues. And I updated Config -> Now it is similar as is crypto/tls. But for now it supports only one certificate.

I have now problem to resolve issue of merge conflicts. I cannot reproduce problem on my computer...

Sean-Der and others added 3 commits November 23, 2019 00:34
Update go.mod and imports for /v2
Breaking changes:
* certificate, privateKey was replaced
  by certificate (tls.Certificate)
* verifyPeerCertificates uses array of bytes for chain
  certificate instead of certificate(*x509.Certificate)

Resolves #53
Use new option in golangci-lint `exclude-use-default` so we can
lint our examples and main code at the same time.
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.

5 participants