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

Revert "Replacing crypto/ed25519 with compatible wrapper" #158

Closed
wants to merge 2 commits into from

Conversation

rumpelsepp
Copy link
Member

This reverts commit 0958495.

As discussed here: #154 (comment)

This package uses conditional compiling for go1.13, see here:
https://github.com/golang/crypto/tree/master/ed25519

Once there is 1.14 out, it won't use the stdlib anymore if the wrapper
itself is not updated. I would recommend to rely only on the stdlib
here. Also, this change is pretty much useless, since the crypto/x509
methods do not support ed25519 keys below go 1.13:

https://pkg.go.dev/crypto/x509@go1.12.13?tab=doc#MarshalPKCS8PrivateKey
https://pkg.go.dev/crypto/x509?tab=doc#MarshalPKCS8PrivateKey

README.md Outdated Show resolved Hide resolved
@rumpelsepp rumpelsepp force-pushed the ed25519 branch 4 times, most recently from cf8dca3 to 8faf01a Compare December 9, 2019 13:34
Copy link
Member

@daenney daenney left a comment

Choose a reason for hiding this comment

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

Please remove the change to the commit linter. These are shared between Pion projects and we'd like those to stay consistent. Though it might be a bit annoying, we're not going to change them right now.

Aside from that, 50 characters is the expected length of the subject:

The defaults are 50 characters for the summary and 72 characters for the description

@rumpelsepp
Copy link
Member Author

rumpelsepp commented Dec 9, 2019

Aside from that, 50-52 characters is the expected length of the subject:

The defaults are 50 characters for the summary and 72 characters for the description

The 50 char subject length is annoying, since git generates sometimes longer subjects, e.g. git revert or git merge. Anyway, the commit is now dropped.

edit: I am starting to get angry against this check… I started to reformat links whick are included in my commit message.

@rumpelsepp rumpelsepp force-pushed the ed25519 branch 3 times, most recently from dbecd48 to 6ee0faf Compare December 9, 2019 13:58
@rumpelsepp
Copy link
Member Author

rumpelsepp commented Dec 9, 2019

Removed the commit description. Hope it builds now, I hate this check.

Finally. IMO the commit message carries less information than before, but at least the script is happy. This check should be relaxed, it is not even possible to include links to github discussions in the body of the message… Also, offline linting seems complicated.

ed25519 key support is not available below go 1.13
@codecov-io
Copy link

Codecov Report

Merging #158 into master will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
- Coverage   73.46%   73.34%   -0.12%     
==========================================
  Files          58       58              
  Lines        3335     3335              
==========================================
- Hits         2450     2446       -4     
- Misses        618      620       +2     
- Partials      267      269       +2
Impacted Files Coverage Δ
crypto.go 56.92% <ø> (ø) ⬆️
config.go 100% <ø> (ø) ⬆️
util.go 87.25% <ø> (ø) ⬆️
conn.go 79.57% <0%> (-0.96%) ⬇️

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 b4351bb...58d9264. Read the comment docs.

@Sean-Der
Copy link
Member

Sean-Der commented Dec 9, 2019

@rumpelsepp really sorry about the commit message stuff, it is garbage (and my fault :( )

I want to move to Conventional Commits but just haven't had the time. I want to generate changelogs and give users nice commit logs to audit. People have asked for it multiple times.

We do need to have the linting because people would push commit with messages like fix issue it was very frustrating at the time!


Is having this code in blocking anything/anyone? Currently we have a few users on 1.12, and reverting this will break them again.

@rumpelsepp
Copy link
Member Author

@rumpelsepp really sorry about the commit message stuff, it is garbage (and my fault :( )

Never mind. It was a bit annoying, but I finally made it. :-)

Is having this code in blocking anything/anyone? Currently we have a few users on 1.12, and reverting this will break them again.

We could also delay this a bit and e.g. make a policy that the two most recent go versions should be supported. Right now this would mean go 1.13 and 1.12. As go has a pretty well defined release cycle it is also predictable for folks using this library.

@daenney
Copy link
Member

daenney commented Dec 9, 2019

The problem with users currently on Go 1.12 is that we don't actually support them. go.mod says Go 1.13, which means we can already be using things introduced in Go 1.13 that wouldn't work for Go 1.12 users, and that'll blow up at compile time.

The reason for #154 as far as I can see is #136, which introduced support for ED25519 but broke Go 1.12 compatibility. CI never caught it, since we don't test on anything but latest stable. This now reverts #154, essentially bringing us back to the state of #136 as far as possible Go 1.12 support goes, basically one month ago. No release has been tagged for any of this, so assuming people aren't tracking master, we're not/yet broken for Go 1.12 users.

I think this makes it clear we need to adopt an actual policy here on what to do, b/c we can't keep going back and forth and breaking the library on people like this.

The suggestion of supporting both Go 1.12 and Go 1.13, and merging this once Go 1.14 comes out makes sense to me. If people are happy with that we can leave this one open. I'll submit a separate PR to ensure we run CI for the 2 Go versions we support so we can catch this kind of issue before it gets merged in the future.

@rumpelsepp
Copy link
Member Author

rumpelsepp commented Dec 9, 2019

From: https://github.com/golang/go/wiki/Go-Release-Cycle#release-maintenance

Minor releases to address non-security problems for Go 1.x stop once Go 1.x+1 is released.

Minor releases to address security problems for Go 1.x stop once Go 1.x+2 is released. For more about security updates, see the security policy.

That being said, even the three most recent releases are feasible. For now this is 1.11, 1.12, 1.13. Personally, I think Go 1.x+1 is enough, the decision is yours.

I'm a monkey.

@Sean-Der
Copy link
Member

nothing to add here :) thanks @rumpelsepp and @daenney for taking time out to maintain/work on this stuff I really appreciate it! It is a good way to end the night reading this thread.

I am going to explore moving to GitHub actions (and fixing some of our tech debt around CI) this weekend! I will make a list on the wiki of everything we want. The big ones are a better commit message linter and testing on windows.

daenney added a commit that referenced this pull request Dec 10, 2019
Based on discussions in #158, we've decided to support the latest 2 Go
versions, so current release and current release -1.

In order to not run the linter twice, limit that to run on the latest
version we support.
@daenney daenney mentioned this pull request Mar 22, 2020
4 tasks
daenney added a commit that referenced this pull request Mar 28, 2020
With Go 1.14 out and the x/crypto/ed25519 wrapper not updated to
fallthrough to the stdlib implementation we've decided to drop Go
1.12 supprot and move to stdlib imports entirely.

Fixes #158
@daenney daenney closed this in d80c2e8 Mar 28, 2020
@rumpelsepp rumpelsepp deleted the ed25519 branch March 28, 2020 17:09
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.

4 participants