veracruz
veracruz copied to clipboard
There may be problems how we authenticate principals
Describe the bug The following code in session-manager/src/session.rs is problematic:
let mut client_id = 0;
for principal in self.principals.iter() {
if principal.certificate() == &peer_certs[0] {
client_id = principal.id().clone();
}
}
If none of the principal
certificates matches the peer_cert[0]
, the code will result in client_id
equal to 0, which is a valid client_id
. Now, since the TLS server will only get to this point after authenticating that the client is a valid participant in the computation, this may not be a real problem.
However, it should be rewritten to something like:
let client_id = {
for principal in self.principals.iter() {
if principal.certificate() == &peer_certs[0] {
principal.id().clone()
}
}
return Err(SessionManagerError::InvalidSomethingOrOther);
};
To ensure that if the TLS server is misconfigured somehow in the future, we do not let unauthorized principals to act as the client with ID 0. To Reproduce It has not manifested, and it may not manifest, but good practice presumes we will change it.
Expected behaviour If a principal certificate does not match, an error should be returned.
Can I work on this ? @dreemkiller I think you just solved it, I just want to explore the code while applying the changes 😅
Since @ShaleXIONG is handling the Outreachy participants, I will let him decide if this is appropriate. Working on it will require that you have an full development environment up and running, which is not a requirement for the work we're thinking of for Outreachy. I have no issue with you working on this, myself.
@Mo-Fatah Hi, we have no problem with you working on this. Yet as Derek mentioned, it may require you to have a relatively deep understanding of the Veracruz project. If you like to focus on adding examples. which may be easier if you have limited knowledge on rust and Veracruz, you can try #384
Great, Thank you!