Closed Bug 1328791 Opened 7 years ago Closed 7 years ago

network.negotiate-auth.trusted-uris doesn't correctly parse when starting with a period (.)

Categories

(Core :: Networking: HTTP, defect)

45 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: trs80, Assigned: mayhemer)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [necko-active])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20161213204132

Steps to reproduce:

I'm trying to get https://docs.microsoft.com/en-us/azure/active-directory/connect/active-directory-aadconnect-sso working with Firefox on Linux and Mac, it works on Windows. On all three platforms I set network.negotiate-auth.trusted-uris to "https://autologon.microsoftazuread-sso.com,https://aadg.windows.net.nsatc.net"

I have successfully used negotiate auth with another local website (with the URL in network.negotiate-auth.trusted-uris) on Linux/Mac


Actual results:

On Windows, I enter my email address at https://portal.office.com and it sends a GET request to https://autologin.microsoftazuread-sso.com/stuff which replies with a 401 Unauthorized with WWW-Authenticate: Negotiate, then Firefox retries the GET with Authorization: Negotiate headers which are accepted and login continues.

On Linux/Mac, after the 401 Firefox doesn't retry the request, and so automatic login doesn't happen.


Expected results:

Firefox on Linux/Mac should retry request after receiving the 401 response. This is possibly similar to 751552?
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
James, can I ask you for logs?  Use about:networking and its "Logging" section.  Alternatively, see [1] for setup with environment variables.

The log modules I'm interested in are:

timestamp,sync,nsHttp:5,negotiateauth:5

Please provide logs from both windows and linux (or osx) - i.e. a success case and a failure case.  Since the logs may contain some private data, feel free to send them directly to my bugzilla email.

Thanks!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging
Assignee: nobody → honzab.moz
Flags: needinfo?(trs80)
Whiteboard: [necko-active]
Hmm, I can now get it to work on Mac, but the order of entries in network.negotiate-auth.trusted-uris matters. In particular, if I have it set to "https://autologon.microsoftazuread-sso.com,https://aadg.windows.net.nsatc.net,.ccgs.wa.edu.au" then it works, but if I have it set to ".ccgs.wa.edu.au,https://autologon.microsoftazuread-sso.com,https://aadg.windows.net.nsatc.net" then it doesn't work for a site on .ccgs.wa.edu.au (https://erm.ccgs.wa.edu.au/ermonline/ in particular).

I tested it on Linux (in a new profile, since I was doing it remotely) and it works too, with the same caveat about ordering. I'll double-check on my desktop with my main profile tomorrow.
(In reply to James Andrewartha from comment #2)
> Hmm, I can now get it to work on Mac, but the order of entries in
> network.negotiate-auth.trusted-uris matters. In particular, if I have it set
> to
> "https://autologon.microsoftazuread-sso.com,https://aadg.windows.net.nsatc.
> net,.ccgs.wa.edu.au" then it works, but if I have it set to
> ".ccgs.wa.edu.au,https://autologon.microsoftazuread-sso.com,https://aadg.
> windows.net.nsatc.net" then it doesn't work for a site on .ccgs.wa.edu.au
> (https://erm.ccgs.wa.edu.au/ermonline/ in particular).
> 
> I tested it on Linux (in a new profile, since I was doing it remotely) and
> it works too, with the same caveat about ordering. I'll double-check on my
> desktop with my main profile tomorrow.

James, this is a great catch, thanks!  Please let me know if this is platform-independent.  If so, then it's probably just a stupid glitch in the preference value parser (split by ',').
I tested on Windows and it has the same behaviour. A quick look through the source for the pref brings me to https://dxr.mozilla.org/mozilla-central/source/extensions/auth/nsHttpNegotiateAuth.cpp#164 which then calls nsHttpNegotiateAuth::TestPref https://dxr.mozilla.org/mozilla-central/source/extensions/auth/nsHttpNegotiateAuth.cpp#624 which has logic I'm not prepared to reason about after midnight (as it is here).
(In reply to James Andrewartha from comment #4)
> I tested on Windows and it has the same behaviour. A quick look through the
> source for the pref brings me to
> https://dxr.mozilla.org/mozilla-central/source/extensions/auth/
> nsHttpNegotiateAuth.cpp#164 which then calls nsHttpNegotiateAuth::TestPref
> https://dxr.mozilla.org/mozilla-central/source/extensions/auth/
> nsHttpNegotiateAuth.cpp#624 which has logic I'm not prepared to reason about
> after midnight (as it is here).

Yep, that is the place.  Thanks.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(trs80)
By the way, my actual initial problem on Linux was NoScript was blocking autologon.microsoftazuread-sso.com, and when I unblocked it it worked fine. So really it's just since then I found the problem with the pref parsing, so I'll retitle the bug.
Summary: Firefox on macOS/Linux doesn't perform negotiate auth after getting a 401 → network.negotiate-auth.trusted-uris doesn't correctly parse when starting with a period (.)
- strchr code style migrated to Tokenizer
- added full support for ipv6 literals
- the code for nego and ntlm are identical (a copy)
- tested only locally with various strings
- fixes a leak of the preference value when match is found
- fixes port -1 that won't match when the pref is "http://host:80"
Attachment #8838232 - Flags: review?(valentin.gosu)
Depends on: 1340252
Comment on attachment 8838232 [details] [diff] [review]
v1 (migrated to Tokenizer)

Review of attachment 8838232 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have unit tests for TestPref? Or at least for MatchesBaseURI? If not, could you file a bug to write some gtests for it?
Also, it would be nice to avoid this code duplication, and it would make it easier for testing to have the implementation in a single place. We might as well do it in this bug.

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +703,5 @@
> +  // The ipv6 literals MUST be enclosed with [] in the preference.
> +  bool ipv6 = false;
> +  if (token.Equals(mozilla::Tokenizer::Token::Char('['))) {
> +    nsDependentCSubstring ipv6BareLiteral;
> +    if (!t.ReadUntil(mozilla::Tokenizer::Token::Char(']'), ipv6BareLiteral)) {

I assume bug 1340252 is supposed to deal with the fact that this currently doesn't capture the [ before the ipv6 literal.
Attachment #8838232 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #8)
> Comment on attachment 8838232 [details] [diff] [review]
> v1 (migrated to Tokenizer)
> 
> Review of attachment 8838232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have unit tests for TestPref? Or at least for MatchesBaseURI? 

Neither.

> If not,
> could you file a bug to write some gtests for it?

I'll file it, but definitely won't work on it ;)

> Also, it would be nice to avoid this code duplication, and it would make it
> easier for testing to have the implementation in a single place. We might as
> well do it in this bug.

Followup as well.  I don't know from scratch how to avoid it, two different modules.  I've spent too much time on this already.

> 
> ::: extensions/auth/nsHttpNegotiateAuth.cpp
> @@ +703,5 @@
> > +  // The ipv6 literals MUST be enclosed with [] in the preference.
> > +  bool ipv6 = false;
> > +  if (token.Equals(mozilla::Tokenizer::Token::Char('['))) {
> > +    nsDependentCSubstring ipv6BareLiteral;
> > +    if (!t.ReadUntil(mozilla::Tokenizer::Token::Char(']'), ipv6BareLiteral)) {
> 
> I assume bug 1340252 is supposed to deal with the fact that this currently
> doesn't capture the [ before the ipv6 literal.

More or less.
Depends on: 1340260
(In reply to Honza Bambas (:mayhemer) from comment #9)
> I'll file it, but definitely won't work on it ;)

Assign it to me then :) I wouldn't like it if we forget about it, and we regress it in the future :)
(In reply to Valentin Gosu [:valentin] from comment #10)
> (In reply to Honza Bambas (:mayhemer) from comment #9)
> > I'll file it, but definitely won't work on it ;)
> 
> Assign it to me then :) I wouldn't like it if we forget about it, and we
> regress it in the future :)

Done.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5610f4305cc
Fix trusted uri list parser for Negotiate/NTLM auth. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b5610f4305cc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Before this bug fixed, I could use 'foo.example.com' to match 'baz.foo.example.com', but now I have to rewrite it to '.foo.example.com'.
If this is intended spec change, is it needed document update?
(In reply to KENZ (Kenji Irie) from comment #15)
> Before this bug fixed, I could use 'foo.example.com' to match
> 'baz.foo.example.com', but now I have to rewrite it to '.foo.example.com'.
> If this is intended spec change, is it needed document update?

I believe the intention was to mach the host exactly when it didn't start with a dot.  So we might actually fix a bug making to explicitly put dot at the start of the host name to match subdomains too.

I'm not entirely sure, tho.
Keywords: dev-doc-needed
I too experienced a regression in behavior. I could use 'example.com' to match 'foo.example.com' or 'bar.example.com' prior to this change. 

This was useful for my (large) corporate environment where 'example.com' is my company name, and 'foo.example.com' is one identity provider within my organization, and 'bar.example.com' is another identity provider with my organization. 

I have tested that prefixing with a dot e.g '.example.com' provides the same behavior as before, but as I understand the original bug, this configuration would break users prior to this change. This would result in a config that only works prior to a specific version and configuration that only works post a specific version.
Depends on: 1351301
You need to log in before you can comment on or make changes to this bug.