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

generate_nmc_cert: rebase against Go 1.8.3 standard library. #54

Merged
merged 6 commits into from
Jun 27, 2018

Conversation

JeremyRand
Copy link
Member

@JeremyRand JeremyRand commented Nov 9, 2017

Not yet tested; feel free to review/test but do not merge.

Ready for review, I think it's mergeable.

@JeremyRand
Copy link
Member Author

Note that I'm NACKing the gofmt -s changes that gometalinter is asking for in this PR, because they needlessly clutter the patchset against upstream. I'll probably have to mess with the Travis script to disable the gofmt linter for the generate_nmc_cert package (and probably also the x509 package).

In Go stdlib, RSA2048 is used by default.  RSA support was removed in our fork, but we neglected to set a default ECDSA curve, so the user had to choose a curve.  P256 is recommended by the Go devs and by us, so it seems to be a reasonable default.
This is because following its recommendations would produce unnecessary noise in the diff against upstream.
@JeremyRand JeremyRand closed this Nov 20, 2017
@JeremyRand JeremyRand reopened this Nov 20, 2017
@JeremyRand
Copy link
Member Author

No idea why the last 2 Travis builds stalled out; triggered a rebuild on Travis in the hope that it was a temporary issue that's fixed now.

@JeremyRand JeremyRand changed the title (WIP) generate_nmc_cert: rebase against Go 1.8.3 standard library. generate_nmc_cert: rebase against Go 1.8.3 standard library. Nov 27, 2017
@JeremyRand
Copy link
Member Author

Ready for review, I think it's mergeable. (I've verified that the cert rehydrates to the same DER encoding as the original; I haven't actually tried putting the generated cert+key into Nginx or anything like that.)

@hlandau
Copy link
Member

hlandau commented Dec 5, 2017

Please remove commented code.

@JeremyRand
Copy link
Member Author

Please remove commented code.

@hlandau The commented code is deliberate; it makes it more straightforward to rebase against future Go versions. Given that I'm the one who will probably be doing those future rebases, I request that the commented code stay intact.

@JeremyRand
Copy link
Member Author

@hlandau Is this okay to merge?

@hlandau
Copy link
Member

hlandau commented Jun 23, 2018

ACK 986772b

@JeremyRand JeremyRand merged commit 986772b into namecoin:master Jun 27, 2018
JeremyRand added a commit that referenced this pull request Jun 27, 2018
986772b generate_nmc_cert: disable goimports linter. (JeremyRand)
fb709df generate_nmc_cert: Use more standard imports order. (JeremyRand)
cee2b18 generate_nmc_cert: Disable gofmt linter. (JeremyRand)
fca636d generate_nmc_cert: Use P256 curve by default. (JeremyRand)
7263b7a generate_nmc_cert: split falsehost into its own file, which makes auditing merges from upstream Go stdlib substantially easier. (JeremyRand)
deea55b generate_nmc_cert: rebase against Go 1.8.3 standard library. (JeremyRand)

Pull request description:

  ~~Not yet tested; feel free to review/test but do not merge.~~

  Ready for review, I think it's mergeable.

Tree-SHA512: 18fab3d3a335f742d021f6b516681a4e3cc2320443b647d12c52bb3726d8e3c2281e2314ab4014b934eaa93329feb891e02768ff5059acf8bce587f7b901b29a
JeremyRand added a commit that referenced this pull request Aug 7, 2018
9496034 Travis: disable gofmt linter. (JeremyRand)
99cb7d9 Travis: enable goimports static analyzer as critical. (JeremyRand)

Pull request description:

  Depends on #54 and #77 .  Should be mergeable after those are merged, assuming Travis passes at that point.

Tree-SHA512: 174fc0d5b0bd5734703d4e97d04d70389883071fec41436ef6a93b543e449032db8faab2ab0a3b287a3c180504e3a2e64d446c613cc4643016c90e13a4744c7a
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.

2 participants