Skip to content
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

Auth: NonceAuth assumes a nonce is attached to an identifier and is too restrictive #533

Closed
leighmcculloch opened this issue Aug 31, 2022 · 6 comments

Comments

@leighmcculloch
Copy link
Member

The NonceAuth type assumes that a nonce for checking authentication is attached to a single identifier. This is fine in a world where there is only a single authenticated identifier in a invocation, but the design of soroban-sdk-auth is intended to support multiple authenticated identifiers.

When there are multiple authenticated identifiers it's unclear how the nonce should be used when authenticating with each identifier.

I think this highlights a problem with bundling nonce validation and auth together, and I see at least a couple ways this could go, there could be other solutions too.

  1. If every identifier is to have their own nonce, then the nonce should probably be part of the Signature, not part of the function arguments. This would be a very opinionated approach, and it could introduce some very awkward constraints. For example, an identifier wouldn't be able to sign two messages with two different participants and succeed because its nonce would become invalidated or require ordering.

  2. If the nonce is intended to be the nonce of the invocation as a whole, then it should probably not be bundled into the auth logic, and instead should be independent logic. It should really take in probably a list of arguments that form a key, and that key has a nonce that is tracked and incremented. In a lot of our existing example contracts that list of arguments would be a single identifier. In a contract with multiple parties signing it might be multiple identifiers.

I think (2) is what we should explore, and it would involve separating nonce logic from the auth logic, maybe into a separate crate.

cc @jonjove @sisuresh

@leighmcculloch
Copy link
Member Author

Option (2) requires an invocation itself to have a scope or namespace with which the nonce operates within.

For example, for Stellar transactions the nonce's scope is the source account.

We need to define what the nonce's scope is. Is it the sum of all identifiers that are signing? Or is it just one identifier, similar to how a Stellar transaction nonce's scope is only the transactions source account and not every operations source account?

Maybe the authorization library should not have an opinion on this and instead should accept a nonce_scope parameter that defines the scope of the nonce.

@leighmcculloch
Copy link
Member Author

I think what I'm describing is a change to check_auth like this:

pub fn check_auth<N, S: NonceScope<N>>(
    env: &Env,
    nonce_scope: S, // 👈 replaces NonceAuth, and holds the scope instead, and how to get the expected nonce for the scope
    nonce: N, // 👈 the type of nonce does not need to be known by the auth lib
    function: Symbol,
    args: Vec<RawVal>,
    signatures: Vec<Signature>, // 👈 where the signature auths go
)

@sisuresh
Copy link
Contributor

sisuresh commented Sep 2, 2022

What would nonce_scope look like?

I'm not sure if this can be done safely, but check_auth could also just take the nonce data key (as a generic). Then it can read the nonce and verify each signature includes it, and then increment it once at the end. The nonce data key here sounds like your nonce_scope since it'll include all the identifiers that are providing authorization. We considered something similar to this for the simple signing case, but went the NonceForSignature route because the nonce didn't have to be derived from the signature.

@leighmcculloch
Copy link
Member Author

Renaming NonceScope to NonceIter:

pub fn check_auth<N: PartialEq, I: NonceIter<N>>(
    env: &Env,
    nonce_iter: I,
    nonce: N,
    function: Symbol,
    args: Vec<RawVal>,
    signatures: Vec<Signature>, // 👈 where the signature auths go
)
pub trait NonceIter<N: PartialEq> {
    fn peek(&self) -> N;
    fn next(&self) -> N;
}

@leighmcculloch
Copy link
Member Author

I think we should address this problem by doing:

@sisuresh
Copy link
Contributor

Closed by #597

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

No branches or pull requests

2 participants