Feature/updated gateway registration #4885
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
jstuczyn
force-pushed
the
feature/updated-gateway-registration
branch
from
33a2505
to
b3d7c26
Compare
There was a problem hiding this comment.
Looking good! But it's also thousands of lines of changes so ...
Reviewed 20 of 50 files at r1, 53 of 53 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jstuczyn)
common/client-core/gateways-storage/src/backend/mem_backend.rs line 115 at r2 (raw file):
// eh. that's nasty, but it's only ever used for ephemeral clients so should be fine for now... details.shared_key = Arc::new(SharedGatewayKey::Current( SharedSymmetricKey::try_from_bytes(updated_key.as_bytes()).unwrap(),
If one would be nitpicky we should don't unwrap and instead error back up, but then again it looks like this one might be one of those that can in practive never fail?
common/client-core/src/init/helpers.rs line 336 at r2 (raw file):
// we can ignore the authentication result because we have **REGISTERED** a fresh client // (we didn't have a prior key to upgrade/authenticate with) assert!(!auth_response.requires_key_upgrade);
Just double checking so this will never actually happen?
common/gateway-requests/src/registration/handshake/error.rs line 10 at r2 (raw file):
pub enum HandshakeError { #[error("received key material of invalid length: {received}. Expected: {expected}")] KeyMaterialOfInvalidSize { received: usize, expected: usize },
I like this, adding context to errors :)
|
Previously, octol (Jon Häggblad) wrote…
I really hate the existence of this one, but the interface behind saving keys, which provides everything with references, does not fully work with the "in memory storage" that keeps everything by value. I also purposely don't expose any Clone/Copy methods on keys to prevent their misuse, so as a hack I convert it to and back from bytes. The reason I'm unwrapping is this can't possibly fail because it's just feeding its own bytes representation |
|
Previously, octol (Jon Häggblad) wrote…
Yes, because for fresh registrations you will always have the up to date key and this code only ever registers the client. However, maybe rather than doing an assertion, I will instead return an error for when this inevitiably changes because of some accidental changes we're going to make in the future... |
jstuczyn
force-pushed
the
feature/updated-gateway-registration
branch
from
69ab2b7
to
be7f00f
Compare
There was a problem hiding this comment.
Reviewable status: 72 of 74 files reviewed, 5 unresolved discussions (waiting on @jstuczyn and @octol)
common/client-core/gateways-storage/src/backend/mem_backend.rs line 69 at r3 (raw file):
} async fn has_gateway_details(&self, gateway_id: &str) -> Result<bool, Self::StorageError> {
The function is infallible but returns a Result
|
Previously, pronebird (Andrej Mihajlov) wrote…
it's because it's part of the trait method which can fail in other implementations |
There was a problem hiding this comment.
Reviewable status: 72 of 74 files reviewed, 4 unresolved discussions (waiting on @jstuczyn and @octol)
common/client-core/gateways-storage/src/backend/mem_backend.rs line 69 at r3 (raw file):
Previously, jstuczyn (Jędrzej Stuczyński) wrote…
it's because it's part of the trait method which can fail in other implementations
Fair enough.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jstuczyn and @pronebird)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jstuczyn)
There was a problem hiding this comment.
Reviewed 20 of 50 files at r1, 52 of 53 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jstuczyn)
This PR introduces support for aes256-gcm-siv shared keys between clients and gateways.
those changes should be fully backwards compatible. if they're not, there's a bug.
To test (I've briefly done it locally, but some more extensive tests would probably be needed):
From the top of my head, the following have to be tested:
I've done some very brief tests locally, such as new client -> old gateway followed by new client -> new gateway to see key being upgraded but by no means were they exhaustive.
I might be stating the obvious here, but whenever I say "existing client", it also implies the associated gateway also has bunch of data stored on its side (since it needs to have the relevant legacy key on its end)
This change is