-
Notifications
You must be signed in to change notification settings - Fork 70
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
fix: update certificate checking if disable_range_check is set #378
Conversation
Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com>
@mraszyk any chance you can document this code better somehow? I have a really hard time understanding what happens for what reason |
for p in paths { | ||
if !(p == vec![t.clone()] || p[0] == rs) { | ||
// the path has neither the form /time nor /request_status/* | ||
return Err(AgentError::CertificateVerificationFailed()); |
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.
Up until now, the CertificateVerificationFailed
error referred to a specific cause: bls::core_verify()
returned something other than BLS_OK
, meaning the certificate signature didn't match the expected value.
I think it would be helpful to someone diagnosing these errors if we introduced a new error type, CertificateVerificationRejected(reason)
, where reason is an enumeration that gives more information about the reason for the rejection of the certificate. What do you think?
Superseded by this PR. |
The
disable_range_check
condition should check that the call is to the Management Canister, not to an application canister offering a method with the nameprovisional_canister_create_with_cycles
.Moreover, the certification validation should follow the updated Interface Spec.