External keys

[djc]

As an alternative to #212, this goes further in the direction of #205, removing alg and key_pair from CertificateParams and requiring the caller to pass in a reference to them when signing. This seems to have a number of nice properties:

• alg is derived from the passed in KeyPair, so key algorithm mismatch errors can no longer occur
• No need for passing in a signing algorithm when parsing from pre-existing parameters or key pairs
• Should make it easy to support long-lived (remote) key pairs

The main downside as far as I can see is that the top-level API gets a bit more complicated, because generating a KeyPair must now be done by the caller, and for now we force them to pick a signing algorithm. I think we might mitigate this by adding a no-argument constructor like generate_default() (or use generate() for the no-argument variant and generate_for() for the variant that requires an argument).

Generally, this feels like a clear improvement in the API's design to me.

[cpu]

Thanks, I think this is the right direction to be headed 👍

I think we might mitigate this by adding a no-argument constructor like generate_default() (or use generate() for the no-argument variant and generate_for() for the variant that requires an argument).

Maybe we could leave generate as the argument required option and implement Default for providing something that can work without argument to give you back a KeyPair without needing to care about the alg? That feels small enough to tack onto this branch if you were game.

[djc]

Maybe we could leave generate as the argument required option and implement Default for providing something that can work without argument to give you back a KeyPair without needing to care about the alg? That feels small enough to tack onto this branch if you were game.

I did consider implementing Default like this, but it doesn't feel like a good fit? Like, the docs for Default::default() say "Returns the “default value” for a type.". IMO generating a random KeyPair is quite different from yielding "the “default value”.

I'm generally happy to tack something onto this branch but figured we should discuss the design first.

[cpu]

IMO generating a random KeyPair is quite different from yielding "the “default value”.

That's a fair point. I agree that it's probably not a good fit with that in mind.

I'm generally happy to tack something onto this branch but figured we should discuss the design first.

generate for the default and generate_for for a specific alg seem OK to me 👍

[djc]

I'm generally happy to tack something onto this branch but figured we should discuss the design first.

generate for the default and generate_for for a specific alg seem OK to me 👍

Added some commits for this.

I also added a commit that removed the alg field from CertificateRevocationList in favor of relying on the issuer.alg.

[djc]

Rebased on top of #215.

[est31]

Mostly good, mainly concerned about the generate_simple_self_signed function. Ideally we make its API as simple as possible, requiring as little parameters as possible.

[djc]

In response to your feedback, I've changed the initial commit to retain CertifiedKey and kept the top-level rcgen::generate_simple_self_signed() returning CertifiedKey (that is, it generates a KeyPair for the caller and returns the CertifiedKey including the certificate and key pair). I've kept Certificate::generate_self_signed() taking the key_pair as an argument. I feel like this strikes a nice balance of a very approachable high-level API with more composable building blocks.

I'll leave this open for a bit but I'm assuming this is in line with the feedback I've received so far.

[est31]

Great PR, really like it!

[djc]

I noticed that the merge queue strategy is currently set to "Squash and merge". Is that an intentional change from before? Seems unfortunate to squash clean commit histories like this before merging.

[cpu]

I would also prefer to rebase merge.

[est31]

Yeah it was an intentional change (by me) because I felt it was hard to associate the commits on main with the PR they came from. I also don't like having to ask contributors to create a clean history, because not all know how to do that, and some simply don't care. The choices are either not having their contributions, fixing it yourself by pushing to the branch, or having the "bad" history in main (say with merge commits from upstream/main or stuff). The first and last option aren't really great, and the second is not good either (for the maintainers).

I see that the option to rebase merge at the time you put the PR into the merge queue is enabled again. I think that's good.

[est31]

That is, I think we can merge PR with a clean history to main in rebase style, as long as the history is kept clean.

[djc]

I think the meta-point here is that I would have expected you to communicate this change prior to making it, since we're collaborating on maintenance of this crate. That you did this silently (and after we discussed that I and @cpu preferred rebasing in #137) is disappointing.

Yeah it was an intentional change (by me) because I felt it was hard to associate the commits on main with the PR they came from.

On GitHub, this is really straightforward, on a commit page like 84a3053 there's a link to the PR (this one's from #166):

I also don't like having to ask contributors to create a clean history, because not all know how to do that, and some simply don't care.

In my experience (across many different projects) most contributors are open to this. And there is the option to just squash-merge for PRs that only contain a single logical change anyway.

The choices are either not having their contributions, fixing it yourself by pushing to the branch, or having the "bad" history in main (say with merge commits from upstream/main or stuff).

So I would say this is more about setting the default. I can live with the default being squash as long as we can still rebase PRs with clean history -- but I would like some commitment from you that you'll discuss meaningful changes to the repo settings like this before making them.

Edit: looks like the merge queue is completely disabled now?

[est31]

I think the meta-point here is that I would have expected you to communicate this change prior to making it, since we're collaborating on maintenance of this crate. That you did this silently (and after we discussed that I and @cpu preferred rebasing in #137) is disappointing.

I've expressed my preference in #137 for squashes, and was quite surprised that after the merge queue got enabled, only rebase merges were implemented. From my point of view that was silent as well, as in I only noticed that a while after the fact, without there having been explicit communication about it. So after I noticed that, I went and changed the setting back to squashes. In retrospect, I guess that I should have communicated that explicitly back when I did the change, which I think should be a lesson for all of us here.

I don't hold any grudges over this, I just wanted to give you my point of things so that you hopefully understand my point of view and don't develop any grudges either.

looks like the merge queue is completely disabled now?

It wasn't me, I didn't touch any setting in the last 2+ weeks at least.

In my experience (across many different projects) most contributors are open to this.

In my experience the contributors whose changes need input the most are those who are also the least able to do it. Someone who is doing their first steps with git, etc. I feel empathy with them and think the bar for contributions should be low so that people can grow. I suppose it depends on the type of project you have though, as some projects attract that group of people more than people who do something for their job (but that is no guarantee, many folks know git only from the GUI). With rcgen I do admit I have seen this more rarely.

Sometimes it's also due to different styles. Some people mainly work on projects that don't allow force pushes or history edits, while other projects explicitly want those.

[djc]

I've expressed my preference in #137 for squashes, and was quite surprised that after the merge queue got enabled, only rebase merges were implemented. From my point of view that was silent as well, as in I only noticed that a while after the fact, without there having been explicit communication about it. So after I noticed that, I went and changed the setting back to squashes. In retrospect, I guess that I should have communicated that explicitly back when I did the change, which I think should be a lesson for all of us here.

I don't hold any grudges over this, I just wanted to give you my point of things so that you hopefully understand my point of view and don't develop any grudges either.

That's fair, we should have made that explicit.

I'm fine with keeping the default setting to squash, and then we can just bypass the queue if/when we want to rebase?

@cpu any thoughts?

[cpu]

@cpu any thoughts?

In an ideal world I think GitHub would let you set a default strategy for the merge queue but override it at submit time. Unfortunately since that feature doesn't exist I think we're stuck evaluating workarounds. I think it's largely a question of what the default should be.

My intuition is that for this repository the type of contributor who might struggle with maintaining a clean commit history is less common than a more git-savvy contributor. A quick informal survey of the past ~9 PRs by external contributors shows that most are either 1 commit, or have a history I'd consider clean enough for a rebase.

What about flipping the proposal and making the default merge queue strategy rebase-merge but leaving the door open for administrative squash-merge that bypasses the queue when the PR merits it?

I do think it's helpful to consider a variety of contributor styles and experience but I also don't like having to babysit CI tasks to wait to administratively merge work in the common case where it would be suitable for a rebase. The administrative merge route is a big hammer because if you don't wait for those CI tasks to complete you could easily merge code that fails in CI.

[est31]

What about flipping the proposal and making the default merge queue strategy rebase-merge but leaving the door open for administrative squash-merge

I've written about my preferences above, but reading your points of views and statements I'm more open to trying out rebase merges by default.

[cpu]

Sounds good. Thanks for being open to giving it a shot. I'll change the merge queue setting to use rebase merge.

If a contribution arrives from someone that looks like they're struggling to maintain a clean commit history we can either put the time in to help them tidy it, or we can administratively squash merge as circumstances merit.

[cpu]

I'll change the merge queue setting to use rebase merge.

Done: