Enable recovery using a subset of recovery keys #879
Conversation
✅ Deploy Preview for marblerun-docs canceled.
|
daniel-weisse
force-pushed
the
dw/recovery-shamir-secrets
branch
from
94db8d8 to
b31012d
Compare
Signed-off-by: Daniel Weiße <[email protected]>
daniel-weisse
force-pushed
the
dw/recovery-shamir-secrets
branch
from
b31012d to
1df337c
Compare
Signed-off-by: Daniel Weiße <[email protected]>
daniel-weisse
force-pushed
the
dw/recovery-shamir-secrets
branch
from
1df337c to
c418763
Compare
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
| } | ||
| if currentManifest.Config.RecoveryThreshold != updateManifest.Config.RecoveryThreshold { | ||
| a.log.Error("UpdateManifest: Invalid manifest: Recovery threshold cannot be updated") | ||
| return nil, 0, errors.New("recovery threshold cannot be updated") |
There was a problem hiding this comment.
just to be sure: we will be able to allow updating this together with "Allow updating recovery public keys on manifest update", right?
There was a problem hiding this comment.
Yes.
Its currently blocked since changing the threshold would require re-generating the recovery secrets, which is effectively the same as changing the keys
| } | ||
|
|
||
| // If this was the first secret uploaded, just return | ||
| if r.correctSecrets < 2 { |
There was a problem hiding this comment.
Should we compare to the threshold instead?
And we should catch a threshold of 1 in Manifest.Check.
There was a problem hiding this comment.
The threshold is unknown for instances in recovery mode.
We could save this information as non-encrypted data, but I'm unsure how that would impact the security of the state
There was a problem hiding this comment.
Ah yes of course. It's probably not worth then.
| // validateSecretLength checks whether a provided secret has a valid length (either 16 or 32 bytes), | ||
| func (r *MultiPartyRecovery) shamirRecover(secret []byte) (int, []byte, error) { | ||
| uploadedSecretHash := Hash(secret) | ||
| secretsRemaining := len(r.SecretHashMap) - r.correctSecrets |
There was a problem hiding this comment.
can this be based on the threshold instead of the number of all keys?
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
daniel-weisse
force-pushed
the
dw/recovery-shamir-secrets
branch
from
35a7da4 to
aa436d8
Compare
Proposed changes
RecoveryThreshold, to control the number of recovery secrets required to recover the Coordinator's stateRecoveryThresholdnumber of recovery secrets to the CoordinatorAdditional info