Skip to content

Jwt auth#90

Open
harshvardhan-s1 wants to merge 7 commits intomasterfrom
jwt_auth
Open

Jwt auth#90
harshvardhan-s1 wants to merge 7 commits intomasterfrom
jwt_auth

Conversation

@harshvardhan-s1
Copy link
Copy Markdown

No description provided.

#[derive(Debug)]
pub enum JwtAuthToken {
/// Fully-formatted `"Bearer <value>"` ready to insert into gRPC metadata.
Static(tonic::metadata::MetadataValue<tonic::metadata::Ascii>),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we use SensitiveString?

Comment on lines +51 to +54
/// Site ID sent in the `x-site-id` metadata header on every request.
///
/// Supports Vector's `${ENV_VAR}` interpolation, e.g. `site_id = "${SITE_ID}"`.
pub site_id: String,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't seem necessary (because JWT would already have the site-id). May be we should remove this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The disadvantage of having it is that we'll have this failure-mode where one sets incorrect site-id (one that doesn't agree with token).

#[configurable_component]
#[derive(Clone, Debug)]
#[serde(rename_all = "snake_case", tag = "type")]
pub enum JwtTokenConfig {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/JwtTokenConfig/AuthToken/

///
/// When configured, each request carries a bearer token and site ID in its gRPC metadata.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub auth: Option<VectorSinkAuthConfig>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we drop site_id above, we can then simplify this to Option<AuthToken> (proposed name)?

Comment thread src/sinks/vector/mod.rs
NoHost,

#[snafu(display("JWT token unavailable: {}", message))]
JwtTokenUnavailable { message: String },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/Jwt/Auth/

Comment thread src/sources/vector/mod.rs
if let Some(auth) = &self.auth {
let metadata = request.metadata();
let authorization = metadata.get("authorization").and_then(|v| v.to_str().ok());
let site_id = metadata.get("x-site-id").and_then(|v| v.to_str().ok());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is unnecessary, let us kill this.

Comment thread src/sources/vector/mod.rs
let metadata = request.metadata();
let authorization = metadata.get("authorization").and_then(|v| v.to_str().ok());
let site_id = metadata.get("x-site-id").and_then(|v| v.to_str().ok());
auth.validate(authorization, site_id).map_err(|e| match e {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let us not check here, let us build Validator (suggested in a comment above) and hold it in Service and check immediately after the map on line 75 below.

Comment thread src/sources/vector/mod.rs
/// When omitted, all incoming requests are accepted without authentication.
/// See [`JwtAuthConfig`] for full documentation.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub auth: Option<JwtAuthConfig>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Create a wrapper type that also includes member_key suggested above.

Comment thread src/sources/vector/mod.rs
Comment on lines +389 to +445
const TEST_PRIVATE_KEY: &str = "-----BEGIN PRIVATE KEY-----
MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDJ5D7lpMrGJpl7
zCcZ73XqbzBaagaPa9QDoGmypTbOoiysnnmcTHfy+wcP2aBlDTC8aB+7iPdZr0tA
ENdzIQ0/kZFBWCdwqAtQYDyfGuZx9y+3E9I8RFleDqDSwA6aUrSoesC9OBHztebX
0m4T9dAWzn8Vr3CYKVpp4XcYwfX6iWszCm43zv4fCJu/qYX67IvOP8h66OMBZ8s7
A4K15z1n8ScI3R6v6amc94iB7z2B9hdvuoTKk89dF5XGxE1ZVnIzSPr/8/oQQJgG
RaYqQAViy4kPmctW4uaI9ajQPIQe58LpNh1lDw+aLRHO/e0SCqbUNARTLSdSIwNV
3dltWgS9AgMBAAECggEAHPo4NuDYw+kdZYHvaM8QdyYfZBLMv0AkTaL0GNKS08S+
McaLQO5O1x7FrDY5yddDU/+D8nhdvE8nN1pTejBXxPSBS0Y6XvaXrSErAlErm1b1
z8q2BbVvuErUNXugfPD7AiWgTWhjVz4YFIkdCJtjEyrvXa7xM73XvtPAMtsAEcXv
MgeRaZVdIledQUozu72RfPuG0yYWG5j+1W1IjNDcuLvld+RrZZ6JqyedhHMwlsFU
bi1DDGaBvp7jkDr6hDp81dqUVposvq+yw3THoyDnQCNxrSCfDpRkYk7DWJKVD8XS
6GvFHuHfaktzm+KkUHBQAebGn6qM+3QBIOWXZkHBdwKBgQDwhVtLUNnz7LLOlAxH
/IF5WM96DoPilOG548yMt/81Zez9QzgJXhxefhCpl2ZQDUCWr9CFvn+98XFai8jt
voVQMV23AGi6nJJ+jGw9koQUt/uYAxZ4U8tG0KqxVGhmrab1MfTpLp2mQWkJN7y1
Hk5moPHwpQhxW73qlzwR8Ug8FwKBgQDW4nX8ZvFfmyJcrckquh0KMpILe5i+klmd
ENU7TmlQ8Sq1QX2j+w4gOWpUR6/bnij1XeEsI21z10Sv3yEgu2E8V7Cqf9mJX0in
+H5+WpEbTHqgfWhA8wXoZIizRfHDKOsOnhNmTFMBBrcp0zd4V1N1xH+APkw1q3jF
YxnmMAMmSwKBgBH5xYLxffiO/iYWRnyy0HJjQs5ae1zZx6z+63Cw56/z+CxNc8iv
cetV/KTQHeNpuiQI68qzHBT0EIa138R08r21ks10iF86CHDQyd4oLxrlTTZlNK61
hIG8YqVyK4NRAyNcInOy+jFMvi7kLYRTyYQ+DxbvHpxqQN1hhCnLIJztAoGAakX9
zCKtZXc3+1YHk5YQHqb8C6nI1RdUMpXMn1QcSee8E4CcPqk/RzieGaiKlLcX0qHn
ZwjubMgeNEzJ+YIyiMFloi0wzPvO1yPSi3MHKNUeIJllIhoO5ewyn1cMRlTKS6Rq
O8Grm2pS0+CeImot4KSZ2jb1QeXYCOcGPA2qwRkCgYEAnCI12DQuInN8nLEo4qtq
XEgyvUZ0fGaezcmeT4hhY94l0/HXS0D0qXs/f/rvfFFnvRYlEyiycA4pClkNRNkM
TM9RBaFTEKw9NQP895KUx6hHIAM/LB1Qyf7cDixtwf8ly7Gqhx4vU9tCiiDGSr9Z
T+QEb2Rxj5SJ8cGbNr+NAEI=
-----END PRIVATE KEY-----";

const TEST_PUBLIC_KEY: &str = "-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAyeQ+5aTKxiaZe8wnGe91
6m8wWmoGj2vUA6BpsqU2zqIsrJ55nEx38vsHD9mgZQ0wvGgfu4j3Wa9LQBDXcyEN
P5GRQVgncKgLUGA8nxrmcfcvtxPSPERZXg6g0sAOmlK0qHrAvTgR87Xm19JuE/XQ
Fs5/Fa9wmClaaeF3GMH1+olrMwpuN87+Hwibv6mF+uyLzj/IeujjAWfLOwOCtec9
Z/EnCN0er+mpnPeIge89gfYXb7qEypPPXReVxsRNWVZyM0j6//P6EECYBkWmKkAF
YsuJD5nLVuLmiPWo0DyEHufC6TYdZQ8Pmi0Rzv3tEgqm1DQEUy0nUiMDVd3ZbVoE
vQIDAQAB
-----END PUBLIC KEY-----";

fn now_secs() -> u64 {
SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap()
.as_secs()
}

fn make_token(extra: HashMap<&str, serde_json::Value>) -> String {
let mut claims = serde_json::Map::new();
claims.insert("sub".into(), serde_json::json!("test-agent"));
claims.insert("exp".into(), serde_json::json!(now_secs() + 3600));
claims.insert("site_ids".into(), serde_json::json!(["site-123"]));
for (k, v) in extra {
claims.insert(k.into(), v);
}
let key = EncodingKey::from_rsa_pem(TEST_PRIVATE_KEY.as_bytes()).unwrap();
encode(&Header::new(Algorithm::RS256), &claims, &key).unwrap()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

move this to src/test_util/auth.rs or some such?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, there is another test-caller above that needs it, let us dedup these

Comment thread src/sources/vector/mod.rs
}

#[tokio::test]
async fn valid_token_and_site_id_delivers() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let us convert these tests to use a mix of keys (eg. site_id, site, foo etc) and also add new test-cases for precedence (eg. when an event has both site_id and foo, the former is used) etc

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