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

Authorization Setup for Write Access on RPC Calls #620

Merged
merged 14 commits into from
Oct 16, 2020

Conversation

flodesi
Copy link
Contributor

@flodesi flodesi commented Aug 11, 2020

Summary of changes
Changes introduced in this pull request:

  • Create JWT write access to required RPC calls
  • Implement auth commands to create and verify JWT tokens
    • This includes the implementing RPC and CLI commands for auth

Reference issue to close (if applicable)

Closes #592

@flodesi flodesi changed the title Jaden/rpc auth Authorization Setup for Write Access on RPC Calls Aug 12, 2020
utils/auth/src/lib.rs Outdated Show resolved Hide resolved
utils/auth/src/lib.rs Outdated Show resolved Hide resolved
@flodesi flodesi requested a review from ec2 August 17, 2020 21:40
forest/src/cli/auth_cmd.rs Outdated Show resolved Hide resolved
node/rpc/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 32 to 44
lazy_static! {
/// Constants of all Levels of permissions
pub static ref ADMIN: Vec<String> = vec![
"read".to_string(),
"write".to_string(),
"sign".to_string(),
"admin".to_string()
];
pub static ref SIGN: Vec<String> =
vec!["read".to_string(), "write".to_string(), "sign".to_string()];
pub static ref WRITE: Vec<String> = vec!["read".to_string(), "write".to_string()];
pub static ref READ: Vec<String> = vec!["read".to_string()];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, none of this needs to be allocated, and why are they kept seperate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, they don't need to be separate, was just matching lotus

Comment on lines +87 to +88
// TODO temp use of bls key as placeholder, need to update keyinfo to use string instead of keyinfo
// for key type
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would string be needed? I don't see how that helps this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In lotus the key_info is not necesarily either BLS or SECP256K1, so switching to a string key_type would allow storing more types of keys (like the JWT secret key) rather than just BLS or SECP256K1.

@austinabell
Copy link
Contributor

I definitely want to think about this more in depth soon because I don't see how this helps us or even has support for multiple tokens (did I miss that part in this PR?)

Also, can you please try running our node and testing functionally, I am getting this error when running and stopping the node:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: IO("File exists (os error 17)")', forest/src/daemon.rs:131:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@flodesi
Copy link
Contributor Author

flodesi commented Aug 20, 2020

I definitely want to think about this more in depth soon because I don't see how this helps us or even has support for multiple tokens (did I miss that part in this PR?)

This PR basically just lays the ground work for adding privileges on certain RPC calls, just like how lotus has set up. as for multiple tokens, the format is the same as with lotus, each token is identical for each permission level, as long as the private JWT secret key is deleted.

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

utack

Comment on lines +16 to +22
pub const ADMIN: [&str; 4] = ["read", "write", "sign", "admin"];
/// Signing permissions
pub const SIGN: [&str; 3] = ["read", "write", "sign"];
/// Writing permissions
pub const WRITE: [&str; 2] = ["read", "write"];
/// Reading permissions
pub const READ: [&str; 1] = ["read"];
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a lot better (enums with a value of priority or something) but can always change later I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think this was from Jaden's existing implementation. I think can be just changed later for now

@StaticallyTypedAnxiety StaticallyTypedAnxiety merged commit 982738c into main Oct 16, 2020
@StaticallyTypedAnxiety StaticallyTypedAnxiety deleted the jaden/rpc-auth branch October 16, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup authorization for 'write' access RPC calls
6 participants