Implement encryptor in keystore library #1587
Conversation
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
✅ API Diff Results - No breaking changes |
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
had a problem deploying
to
integration
GitHub Actions
Error
connorwstein
had a problem deploying
to
integration
GitHub Actions
Error
connorwstein
had a problem deploying
to
integration
GitHub Actions
Error
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
| } | ||
| return EncryptResponse{EncryptedData: encrypted}, nil | ||
| default: | ||
| return EncryptResponse{}, ErrEncryptionFailed |
There was a problem hiding this comment.
Should these ErrEncryptionFailed errors be wrapped with more context? Currently there is no difference between too long, unknown type, name not found, etc.
There was a problem hiding this comment.
yeah thats intentional - most encrypt/decrypt libs operate that way since sometimes leaking info like "bad padding" can lead to attacks. That being said probably less important if the code is OS, not sure can ask sec what they think
| } | ||
| sharedSecret, err := curve25519.X25519(internal.Bytes(key.privateKey), req.RemotePubKey) | ||
| if err != nil { | ||
| return DeriveSharedSecretResponse{}, ErrSharedSecretFailed |
There was a problem hiding this comment.
We should include the original error, no? Or at least log it if we don't want to expose something?
connorwstein
had a problem deploying
to
integration
GitHub Actions
Error
connorwstein
had a problem deploying
to
integration
GitHub Actions
Error
connorwstein
had a problem deploying
to
integration
GitHub Actions
Error
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
* Wip * Basic signer test * Errors new * Fix merge * Remove unnecessary lock
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
| const ( | ||
| // Maximum payload size for encrypt/decrypt operations (100kb) | ||
| // Note just an initial limit, we may want to increase this in the future. | ||
| MaxEncryptionPayloadSize = 100 * 1024 |
There was a problem hiding this comment.
Do we allow users to pass in a limit when creating the keystore? Agree this is fine for now, but since we're going to be trusting clients to use this correctly, it would make sense for them to be able to provide the limit. (Maybe we can follow the OCR model of having a MaxMax limit)
There was a problem hiding this comment.
We don't yet but yep we can add a user facing limit with an underlying MaxMax
| k.mu.RLock() | ||
| defer k.mu.RUnlock() | ||
|
|
||
| if len(req.EncryptedData) == 0 || len(req.EncryptedData) > MaxEncryptionPayloadSize*2 { |
There was a problem hiding this comment.
Is this potentially brittle? Can we document how we got to this number and under what circumstances it would change
There was a problem hiding this comment.
yeah I think it'd be possible in principle that we introduce some new scheme which has enormous overhead/envelope size (more than 2x), but exceedingly unlikely. Perhaps better to have explicit envelope sizes per type (at least the existing ones so far are fixed so we can start with that). Can do in a follow up
connorwstein
had a problem deploying
to
integration
GitHub Actions
Failure
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
connorwstein
temporarily deployed
to
integration
GitHub Actions
Inactive
5 participants
Note signer impl already reviewed.