-
Notifications
You must be signed in to change notification settings - Fork 24
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
Iot Config service use Helius for on-chain metadata #428
Conversation
impl_msg_verify!(SessionKeyFilterListReqV1, signature); | ||
impl_msg_verify!(SessionKeyFilterStreamReqV1, signature); | ||
impl_msg_verify!(SessionKeyFilterUpdateReqV1, signature); | ||
impl_msg_verify!(iot_config::OrgCreateHeliumReqV1, signature); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've started pathing these message types here since they will have very similar names between other services (like the mobile config service)
9a82be5
to
472d8e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial first pass, will need to take a second run
This is ready to review it's in "draft" status to prevent merging before updating the dependencies once they get updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this lgtm, my eyes hurt from all the scrolling but other than that lg...
Once merged i may tweak a few things on the iot verifier side
} | ||
|
||
impl AuthCache { | ||
pub async fn new( | ||
settings: &Settings, | ||
db: impl sqlx::PgExecutor<'_> + Copy, | ||
) -> Result<Self, AdminAuthError> { | ||
) -> anyhow::Result<(watch::Sender<CacheKeys>, Self)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not import result in line 2? I notice you import in some modules but not others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a stylistic choice that i'm up for debate over; i don't feel overly strongly about it, but given the std::result::Result
is part of the typical prelude unless you override it with use anyhow::Result
then it's easier at a glance, particularly in a large file that what you're returning is not the std Result type
signature: vec![], | ||
}; | ||
request.signature = self.signing_key.sign(&request.encode_to_vec())?; | ||
tracing::debug!(pubkey = address.to_string(), "fetching gateway info"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure what value this log statement provide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably a better place for a metric on grpc requests then a log statement, good point
use helium_crypto::PublicKeyBinary; | ||
use sqlx::{PgExecutor, Row}; | ||
|
||
pub struct IotMetadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slightly confusing having both GatewayMetadata and IotMetadata structs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had hoped scoping the IotMetadata
struct inside the db
submodule would help with that, but i couldn't think of a better way to do unless i skipped the FromRow
impl altogether; since the GatewayInfo
requires the hextree map to look up a Region from the asserted location and i can't change the from_row
function signature to pass that in i needed the intermediary data structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand andymck's point, but I can't think of a better way either and I think scoping IotMetadata
to the db
submodule does help.
c453021
to
86df017
Compare
signer: signer.clone(), | ||
signature: vec![], | ||
}; | ||
futures::future::ready(signing_key.sign(&update_res.encode_to_vec())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I need to remember this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple Nits, but otherwise nice job!!!
Co-authored-by: andymck <andrewb.mckenzie@gmail.com>
86df017
to
7d7edfe
Compare
71fda35
to
c8cde12
Compare
Co-authored-by: Matthew Plant <matty@nova-labs.com>
c8cde12
to
b0c265b
Compare
Switches the iot-config service (and the rest of oracles) to use the helius database for retrieving on-chain gateway metadata instead of the blockchain-node follower client service.
This also adds additional optimizations for signature verification of sent and received messages with the iot-config service, primarily enforcing messages include the binary of the signing pubkey for O(1) verification of the signature verification instead of O(n) when checking the signature against all potentially valid signing keys; instead of attempting to verifying the signature of a message against all potential keys we validate against the key as self-reported in the message and then simply check for membership of that key in the list of potential authorized signers. This also introduces additional message signing of outgoing messages sent by the config service itself
Depends on: