Skip to content

feat: response authentication (certificate verification, root key lookup, delegations)#86

Merged
hansl merged 16 commits intonextfrom
es-verify-cert-fetch-root-key
Nov 30, 2020
Merged

feat: response authentication (certificate verification, root key lookup, delegations)#86
hansl merged 16 commits intonextfrom
es-verify-cert-fetch-root-key

Conversation

@ghost
Copy link

@ghost ghost commented Nov 19, 2020

Implements from doc:

  • Checking BLS signature
  • Fetching root key from /api/v1/status for dev instances
  • Hard-coding Sodium key (still need the actual key). (Note doc says this would be the Mercury key)
  • Support for delegations

See also companion PR sdk #1197


static INIT_BLS: Once = Once::new();

// todo: do not merge until this is the actual Sodium key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

}
}

fn initialize_bls() -> Result<(), AgentError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is getting big, does it make sense to move this, extract_der, and the lookup functions to a different file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

DerPrefixMismatch { expected: Vec<u8>, actual: Vec<u8> },

#[error("The status response did not contain a root key")]
NoRootKeyInStatus(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also print the status here (may be useful)

}

/// Fetch the root key from the status endpoint.
/// It is not necessary to call this when communicating with "the" Internet Computer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe reword to affirm the positive "Only an agent communicating with a local/custom instance of the Internet Computer should call this method. It is not necessary to call this when communicating with the DFINITY foundation managed Internet Computer."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment on lines 163 to 164
let mut write_guard = self.root_key.write().unwrap();
*write_guard = root_key;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut write_guard = self.root_key.write().unwrap();
*write_guard = root_key;
if let Ok(mut write_guard) = self.root_key.write() {
*write_guard = root_key;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting rid of the unwraps

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I applied these changes. The reason I had them as unwrap() is because the RwLock poisoning condition that would cause them to fail can't happen in the code as is, because the writer can't panic.

Comment on lines 169 to 170
let root_key = self.root_key.read().unwrap().clone();
Ok(root_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let root_key = self.root_key.read().unwrap().clone();
Ok(root_key)
if let Ok(read_lock) = self.root_key.read() {
let root_key = *read_lock;
Ok(root_key)
} else {
Err(AgentError::CouldntReadRootKey)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting rid of unwraps and adding some new AgentError

@ghost ghost added the blocked label Nov 20, 2020
@ghost ghost removed the blocked label Nov 30, 2020
@hansl hansl merged commit cc2e5a7 into next Nov 30, 2020
@hansl hansl deleted the es-verify-cert-fetch-root-key branch November 30, 2020 22:55
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 this pull request may close these issues.

2 participants