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

How should authorizers handle key rotation? #150

Open
seh opened this issue Oct 28, 2024 · 11 comments · May be fixed by #154
Open

How should authorizers handle key rotation? #150

seh opened this issue Oct 28, 2024 · 11 comments · May be fixed by #154

Comments

@seh
Copy link
Contributor

seh commented Oct 28, 2024

Say that I have a root private key that I've been using for a while, and now I wish to rotate it, replacing it with a different key that I'll use to sign root blocks. Ideally I'd be able to create that key, and distribute its corresponding public key for use in my authorizers early, getting them ready to start receiving and trusting tokens signed with the new public key. After all of my authorizers have that new public key in use, I'd then be able to swap the root private and start signing token blocks with it.

As it stands today, though, the (*Biscuit).Authorizer method accepts only a single ed25519.PublicKey parameter. That method will return ErrInvalidSignature if any of the blocks in the token aren't signed correctly, but it's hard to tell if it we may have used the wrong root public key, or whether we have the correct root public key, but one of the other blocks in the token suffered tampering.

Have you considered accepting more than one root public key in this method, or in a newly introduced method to sit alongside it? By analogy, within the age library, the age.Decrypt function accepts one or more "identities"—private keys—with which to try to decrypt the file. (The variadic parameter in the signature tolerates a caller supplying zero identities, but the function panics at run time if there aren't any identities available to use.)

@seh
Copy link
Contributor Author

seh commented Oct 29, 2024

@divarvel, would you be amenable to a patch that introduced a sibling method to (*Biscuit).Authorizer that would try multiple root public keys before giving up?

Perhaps it could be called AuthorizerForRootKeys, or something to that effect. I imagine that the existing method would then call on this one with the lone key supplied.

@divarvel
Copy link
Contributor

That’s a great point, the biscuit spec actually takes this use-case into account, it’s just that the go lib does not surface it yet.

biscuit tokens carry an optional key id (an integer) that allows the verifying party to select the expected key within the set of accepted keys.

So in the rust implementation for instance, you can either pass a PublicKey directly, or an Option<u32> -> Result<PublicKey, E>. In go that would amount to func(*int) (*PublicKey, error). The goal is to select the public key beforehand, in order to avoid trying several keys.

I would welcome a patch that exposes this behaviour (not sure it would be possible to have a polymorphic function taking both public keys and closures, but a variant would be fine)

@seh
Copy link
Contributor Author

seh commented Oct 29, 2024

Does your func(*int) (*PublicKey, error) mean that the ID is optional? I suppose so, since you mentioned that a token's ID is optional, so it may not designate any key at all.

That would still leave open the ambiguity as to which public key to try in the face of the rotation scenario that I described. In between—or at the intersection of—these ideas is a projection function with signature func(*int) []PublicKey, allowing it to return the empty set if need be, which would then preclude verification.

While we're here, how does one associate a key ID with a token at minting time? That looks like something that Builder should learn, probably as a builderOption such as WithRootKeyID(int).

@seh
Copy link
Contributor Author

seh commented Oct 29, 2024

Alternately, I could understand taking the stance that if callers fail to provide a key ID, then they're not opting into a tenable rotation approach, so they'd have to suffer calling (*Biscuit).Authorizer (or similar) multiple times in succession until one of the candidate keys suffices.

@divarvel
Copy link
Contributor

While we're here, how does one associate a key ID with a token at minting time? That looks like something that Builder should learn, probably as a builderOption such as WithRootKeyID(int).

yes, it should be added to the biscuit builder (just for the authority block, attenuation blocks are not affected

@divarvel
Copy link
Contributor

Alternately, I could understand taking the stance that if callers fail to provide a key ID, then they're not opting into a tenable rotation approach, so they'd have to suffer calling (*Biscuit).Authorizer (or similar) multiple times in succession until one of the candidate keys suffices.

yes, for seamless key rotation you are expected to include an id in the token

@seh
Copy link
Contributor Author

seh commented Oct 29, 2024

yes, it should be added to the biscuit builder (just for the authority block, attenuation blocks are not affected)

Can you clarify that last point? If we accepted an optional key ID when constructing a Builder via NewBuilder, we'd be able to store it for use in a subsequent call to (*Builder).Build. Were you concerned that we'd accept the key ID when we shouldn't, or wind up including it in the wrong place?

I'm not sure yet whether it's possible or desirable to try using a Builder when attenuating an existing token. That's what I thought you might be alluding to.

@divarvel
Copy link
Contributor

I was not sure whether the same builder could be used to build an authority block and an attenuation block, but it seems it’s two different interfaces, so it’s fine, there is no risk of confusion

@seh
Copy link
Contributor Author

seh commented Oct 31, 2024

That looks like something that Builder should learn, probably as a builderOption such as WithRootKeyID(int).

I wrote this part, but the (*builder).Build method calls on the New function. In order to tunnel the optional root key ID in, I'd need to do one of the following:

  • Add a new required parameter to New, even if of pointer type
    This breaks existing callers.
  • Add a new variadic parameter to New accepting functional options
    Existing callers would still work, unless they're binding the New function to variable of type func(io.Reader, ed25519.PrivateKey, *datalog.SymbolTable, *Block).
  • Introduce an unexported function upon which New could call
    This allows existing callers to continue working.
  • Introduce an exported function upon which New could call
    This allows existing callers to continue working, but requires that we come up with a palatable name for the exported function, and decide whether we want to take on the risk of needing to change this signature again if we introduce yet more paramaters. Accepting functional options would help to alleviate that problem.

I'm inclined to introduce the unexported function first, after which we could decide whether we wish to promote it and commit to it as an exported function.

@seh
Copy link
Contributor Author

seh commented Oct 31, 2024

Please see #151, which pursues the third option proposed in #150 (comment).

@seh
Copy link
Contributor Author

seh commented Dec 17, 2024

Please see #154 for matching more closely to the Rust library's projection from key ID to public key, per #150 (comment).

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 a pull request may close this issue.

2 participants