-
Notifications
You must be signed in to change notification settings - Fork 17
Fix: ACME directory URL redirects are not followed #77
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
Conversation
|
Did not have time to take a good look at this; some general comments:
Something I would want to see here is a simple loop in pub async fn get(&self, url: &Uri) -> Result<http::Response<Bytes>, RequestError> {
let mut u = url.clone();
for _ in 0..MAX_REDIRECTS {
let req = ...;
let res = self.http.request(req).await?;
if res.status().is_redirection() {
u = ...;
continue;
}
return Ok(res);
}
Err(...)
} |
|
Thanks for your input. Your suggestion makes a lot of sense, compared to my previously overengineered solution. I have added some new error options, some input if that works for you, or if I should rather compress them into existing ones would be appreciated. |
bavshin-f5
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! Just one style change and I can merge it.
Please, also squash the changes into a single commit with the next update.
7ae6c30 to
df04c8e
Compare
bavshin-f5
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.
Looks good, thanks!
Partial fix for #75
Technically this PR fixes the general redirect aspects for the GET call in
acme.rs.As of now the POST from
acme.rswould still not be covered:https://github.com/nginx/nginx-acme/blob/main/src/acme.rs#L181
Based on my testing this should only affect use-cases where URLs (relative, or absolute) are returned, that cause another redirect, which should be rarely the case (at least not in the environments I tested with). Reason being, that the URLs provided by
https://acme.example.com/directoryshould normally be OK not requiring redirects.Also I'm not completely sure how to best implement the requirements for
RFC 8555 §6.2.I can add those as well if needed, though it might be best to extract part of the redirect functionality to it's own (reusable) unit I assume.
Any opinions on this matter?
IMO this basic PR should already cover a few additional standard use cases, such as:
https://acme.example.com/directory->https://acme.example.com/some-id/directory(from ACME directory URL redirects are not followed #75)