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

Preemptive authentication does not work when only setting the host #285

Closed
tommix000 opened this issue Oct 26, 2017 · 10 comments · Fixed by #348
Closed

Preemptive authentication does not work when only setting the host #285

tommix000 opened this issue Oct 26, 2017 · 10 comments · Fixed by #348

Comments

@tommix000
Copy link

tommix000 commented Oct 26, 2017

public void enablePreemptiveAuthentication(String hostname)

populates the AuthCache using the port numbers -1 and preemptive authentication does not happen when using the default ports 80/443.

It works by calling Sardine#enablePreemptiveAuthentication(host, 80, 443);

@kkofler
Copy link
Contributor

kkofler commented Jul 18, 2022

Note that this also affects enablePreemptiveAuthentication(URL) when using a URL of the form https://dav.example.com as opposed to https://dav.example.com:443, see the implementation of enablePreemptiveAuthentication(URL) and the Javadoc for URL.getPort() (it returns -1 when not explicitly set).

@lookfirst
Copy link
Owner

There is also this in the documentation.

@lookfirst
Copy link
Owner

I'm going to close this. Not sure this is a real issue. If it is, we can reopen.

@kkofler
Copy link
Contributor

kkofler commented Jul 18, 2022

Uh, this is definitely a real issue in production, which is why I found this issue report. We tried using a URL of the form https://dav.example.com/ without port in our code, and enablePreemptiveAuthentication with that URL had no effect, we still run into https://groups.google.com/g/sardine-dav/c/OF-DUlSZK1E (i.e., exactly what your documentation link says we need to enable preemptive authentication for). We are currently using the "call list first" workaround (because I have just started debugging the issue and found this issue report), but we will try passing an explicit port next to verify that it helps.

If it does (testing still pending here), enablePreemptiveAuthentication(String) and enablePreemptiveAuthentication(URL) need to be fixed to use an explicit default port instead of -1 if the port is not explicitly specified, and/or enablePreemptiveAuthentication(String, int, int) needs to translate -1 to 80/443 before passing it to the HttpHost constructor.

@lookfirst lookfirst reopened this Jul 18, 2022
@lookfirst
Copy link
Owner

@kkofler Sounds good. Thanks for the update. Once you test and find a solution, a PR is much appreciated.

@lookfirst
Copy link
Owner

This is so old, I don't remember why we ended up with -1 there... it does seem like a huge amount of code smell to do that, but I have a feeling it was to work around some issue in HttpClient which may or may not be long resolved. Again, happy to accept a PR if you can figure this all out.

@kkofler
Copy link
Contributor

kkofler commented Jul 18, 2022

It is possible that you might actually need to add cache entries both for -1 and for the real default port, though I hope not.

@kkofler
Copy link
Contributor

kkofler commented Jul 18, 2022

We are going to try:

URL baseURL = …;
int port = baseURL().getPort();
sardine.enablePreemptiveAuthentication(baseURL.getHost(), port >= 0 ? port : 80, port >= 0 ? port : 443);

as a local workaround. If that works out, I will send a PR implementing this (the magic in the ternaries) within sardine.enablePreemptiveAuthentication(String, int, int); if not, I will keep you updated on our findings and we will see how to proceed.

@kkofler
Copy link
Contributor

kkofler commented Jul 19, 2022

We have tested it: The sardine.enablePreemptiveAuthentication(baseURL.getHost(), port >= 0 ? port : 80, port >= 0 ? port : 443); workaround works indeed. So, as I suggested previously, sardine.enablePreemptiveAuthentication(String, int, int) should translate -1 to 80 resp. 443. I am going to submit a pull request.

kkofler added a commit to kkofler/sardine that referenced this issue Jul 19, 2022
In SardineImpl.enablePreemptiveAuthentication(String, int, int,
Charset), do not pass -1 to the HttpHost constructor. The later cache
lookup is always done with an explicit port, so port -1 never matches
and the entry does nothing. Instead, we need to use the default port
explicitly, then it will work, even when connecting with a URL that does
not include the port.

Since the other overloads delegate to this one, this fixes all overloads
to do the right thing, in particular, the
enablePreemptiveAuthentication(String) (hostname-only) and
enablePreemptiveAuthentication(URL) (when using a URL without an
explicit port) ones.

Fixes lookfirst#285.
@kkofler
Copy link
Contributor

kkofler commented Jul 19, 2022

See PR #348, now with correct issue references.

dkocher added a commit that referenced this issue Apr 24, 2024
Fix preemptive authentication without explicit port (#285)
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 a pull request may close this issue.

3 participants