-
-
Notifications
You must be signed in to change notification settings - Fork 76
#419 - Add authentication switches to traffic-gen + docs #439
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
base: main
Are you sure you want to change the base?
#419 - Add authentication switches to traffic-gen + docs #439
Conversation
wez
left a comment
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.
Thanks for this! I think making it a bit simpler is the way to go for now; we don't support other mechanisms yet, and I feel like if/when we do add other mechanisms, we'll want to change the code around a bit to accommodate those.
For now taking out the explicit mechanism selection helps keep things a bit lighter and easier to read.
crates/traffic-gen/src/main.rs
Outdated
| if requested_auth.is_some() && !self.starttls { | ||
| anyhow::bail!("--starttls must be specified when authenticating to SMTP targets"); | ||
| } |
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.
This is actually a server side policy and I think it is overly restrictive to encode this check here. For example, we often find that folks use traffic-gen against their existing non-kumo infrastructure when evaluating and performing comparisons, and those non-kumo deployments may have no such policy in place.
It feels more flexible to me to avoid encoding this sort of policy in this particular client
| if requested_auth.is_some() && !self.starttls { | |
| anyhow::bail!("--starttls must be specified when authenticating to SMTP targets"); | |
| } |
f022de4 to
59fff9d
Compare
|
So... I've simplified the SMTP auth and connection flow as you suggested ( keep it simple crx :) )
hope this keeps the code much cleaner and behaviorally consistent with your feedback |
wez
left a comment
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.
I think we can simplify this a bit more; there's more code than I think is strictly necessary here!
Add a selectable authentication mechanism with username/password options, validate credential handling, and apply those credentials to both SMTP (including STARTTLS re-EHLO and AUTH PLAIN negotiation) and HTTP injection requests in traffic-gen
--authoption (ValueEnum) withplainvalue,--auth-usernameand--auth-password.AUTH PLAINmod smtp_auth:require_auth_mech_in(mechs: &str, mech: AuthMechanism); verify server advertises requested mechdo_smtp_auth(client, mech, mechs, creds); central dispatch for mechanism specific auth logic (currently handles PLAIN)Helpers are intentionally isolated to avoid impacting the HTTP path
auth_credentials()?for reqwestbasic_auth--starttlsand support for HTTP basic authLocally tested with both:
via:
and other combinations