Create URL Protection Space from HTTPURLResponse #2761

lxbndr · 2020-04-05T13:34:54Z

Basic implementation of "Www-Authenticate" parsing. Some real-life responses could crash previous implementation, as it doesn't work with realm in case insensitive way. We decided to modify existing (but unused) internal helper, considering that a good step in iterative improvement of FoundationNetworking.

This also fixes access to "Www-Authenticate" header in some places,
as HTTPURLResponse does capitalize headers after #2728.

lxbndr · 2020-04-05T13:36:02Z

CC @compnerd @spevans @millenomi

spevans · 2020-04-05T17:35:26Z

@swift-ci test linux

spevans · 2020-04-05T20:21:32Z

@swift-ci test linux

spevans · 2020-04-06T18:36:18Z

Sources/FoundationNetworking/URLProtectionSpace.swift

-        let port = response.url?.port ?? 80        //HTTP
-        let _protocol = response.url?.scheme
-        guard let wwwAuthHeader = response.allHeaderFields["Www-Authenticate"] as? String else {
-            fatalError("Authentication failed but no Www-Authenticate header in response")


It might be better to be stricter on the input validation, it can alway be loosened later on. Rather than defaulting values I would suggest:

guard let url = response.url, let host = url.host, let _protocol = url.scheme, _protocol == "http" || _protocol == "https" else { return nil } let port = url.port ?? (_protocol == "http" ? 80 : 443)

to tighten up the validation.

Makes sense. And less code. I like it. Will do.

spevans · 2020-04-06T18:40:52Z

Sources/FoundationNetworking/URLProtectionSpace.swift

+        // Typical Www-Authenticate header is something like
+        //   Www-Authenticate: Digest realm="test", domain="/HTTP/Digest", nonce="e3d002b9b2080453fdacea2d89f2d102"
+
+        let components = authenticateValue.components(separatedBy: " ")


Will this split correctly if there are multiple spaces between each part?
I think split(separator: Character, maxSplits: Int = Int.max, omittingEmptySubsequences: Bool = true) -> [Substring] might be a better choice as by default it will not add empty sequences to the output array.

This is the grammar:

WWW-Authenticate = *( "," OWS ) challenge *( OWS "," [ OWS challenge ] ) auth-param = token BWS "=" BWS ( token / quoted-string ) auth-scheme = token challenge = auth-scheme [ 1*SP ( token68 / [ ( "," / auth-param ) *( OWS "," [ OWS auth-param ] ) ] ) ] credentials = auth-scheme [ 1*SP ( token68 / [ ( "," / auth-param ) *( OWS "," [ OWS auth-param ] ) ] ) ]

where:

OWS = *( SP / HTAB ) ; optional whitespace … BWS = OWS ; "bad" whitespace

so it looks like we need to split for " " or "\t". Use the CharacterSet variants maybe?

https://tools.ietf.org/html/rfc7235#section-4.1 + https://tools.ietf.org/html/rfc7230#section-3.2.3

spevans · 2020-04-06T19:05:28Z

Sources/FoundationNetworking/URLProtectionSpace.swift

+                return NSURLAuthenticationMethodNTLM
+            case "negotiate":
+                return NSURLAuthenticationMethodNegotiate
+            default:


Will it cause problems to create a URLProtectionSpace for methods that are not currently implemented (ie everything except Basic)? Could always return nil for these other ones for now

I actually thought about this helper point as parsing as much info as possible (and reasonable). Hmm, but on the other side it could take responsibility to cut off unsupported things. Agree, this should return nil for everything other than Basic auth.

spevans · 2020-04-06T19:18:14Z

Sources/FoundationNetworking/URLProtectionSpace.swift

+            // Basic implementation.
+            // Doesn't follow complete spec of quoted strings (RFC-7230, p. 3.2.6).
+            // Handles escaping of quotes only.
+


Give that this is concerned with authentication I think that the complete spec should be followed. Maybe it could be broken out into a separate function that fully parses a String into its constituent parts. If any percent decoding needs to be done on the input string then there is DataURLProtocol._PercentDecoder that could possibly be used.

Thanks for hint. I guess I will also look into curl implementation of similar task. AFAIK it is pretty simple. I like the idea of separate parsing, could be also tested separately.

millenomi · 2020-04-06T22:18:55Z

Good by me once the feedback is looked at.

millenomi · 2020-05-11T17:14:47Z

Sources/FoundationNetworking/URLProtectionSpace.swift

+                return port
+            }
+
+            switch scheme {


I forget if scheme is normalized?

This should probably be scheme.lowercased().

millenomi · 2020-05-11T17:18:32Z

Sources/FoundationNetworking/URLProtectionSpace.swift

+        // Typical Www-Authenticate header is something like
+        //   Www-Authenticate: Digest realm="test", domain="/HTTP/Digest", nonce="e3d002b9b2080453fdacea2d89f2d102"
+
+        let components = authenticateValue.components(separatedBy: " ")


This is the grammar:

WWW-Authenticate = *( "," OWS ) challenge *( OWS "," [ OWS challenge ] ) auth-param = token BWS "=" BWS ( token / quoted-string ) auth-scheme = token challenge = auth-scheme [ 1*SP ( token68 / [ ( "," / auth-param ) *( OWS "," [ OWS auth-param ] ) ] ) ] credentials = auth-scheme [ 1*SP ( token68 / [ ( "," / auth-param ) *( OWS "," [ OWS auth-param ] ) ] ) ]

where:

OWS = *( SP / HTAB ) ; optional whitespace … BWS = OWS ; "bad" whitespace

so it looks like we need to split for " " or "\t". Use the CharacterSet variants maybe?

millenomi · 2020-05-11T17:18:48Z

Sources/FoundationNetworking/URLProtectionSpace.swift

+        // Typical Www-Authenticate header is something like
+        //   Www-Authenticate: Digest realm="test", domain="/HTTP/Digest", nonce="e3d002b9b2080453fdacea2d89f2d102"
+
+        let components = authenticateValue.components(separatedBy: " ")


https://tools.ietf.org/html/rfc7235#section-4.1 + https://tools.ietf.org/html/rfc7230#section-3.2.3

lxbndr · 2020-05-11T17:57:30Z

I'm so sorry, the solution is really delayed. It is still WIP. Had done about 3/4. I am thinking to convert this to draft, to not to look like stale PR.

millenomi · 2020-08-26T21:50:57Z

@lxbndr are you still unable to finish this patch?

lxbndr · 2020-08-26T22:30:33Z

@millenomi
I am sorry for delay, again. I've revisited it lately and now think it is ready to be pushed. Will do tomorrow.
It actually blocks me from finishing another auth-related issue, so not going to postpone it anymore.

millenomi · 2020-08-26T22:32:18Z

Thank you! I'll be here to facilitate.

lxbndr · 2020-08-27T16:43:37Z

This all started as an attempt to parse "realm" parameter and not to crash. New implementation is RFC-compliant. I think I found perfect place for parsing code, and it is in _HTTPURLProtocol extensions, where other HTTP-related parsing already lives.

All necessary string comparisons are case-insensitive, but internally data stored as is, so _Challenge structure is more like raw parsed data.

Some simplifications made assuming we are supporting only "Basic" scheme. But in general this code could be used to adopt other schemes.

I reused as much existing code as seemed reasonable (like token parsing, whitespace trimming, character constants).

It seems like we could refactor existing code a bit to improve consistency and streamline some logic. E.g. existing code references RFC-2616, which is obsoleted by RFCs 7230-7235. To keep this patch focused I used old code as is.

The general flow of WWW-Authenticate parsing was inspired by CURL's approach, as it seems pretty robust.

lxbndr · 2020-08-27T16:46:20Z

@millenomi Could you please take a look? I think I took all comments into account. But as changes are much bigger than first time, I am expecting everything.

millenomi · 2020-08-27T16:50:23Z

Nothing immediately jumps at me. At this point, this looks large and useful enough that we can start taking bug fixes as separate patches if needed.

millenomi · 2020-08-27T16:50:30Z

@swift-ci please test

lxbndr · 2020-09-25T16:39:50Z

I found that one line I've changed was unnecessary. Reverted HTTPURLProtocol.swift:137 to original.
No other changes were made.

@millenomi Is there anything else I can do here? I am not in a rush, just thought maybe I missed some requirement.

xwu · 2020-09-25T21:06:41Z

Sources/FoundationNetworking/URLSession/Message.swift

@@ -111,6 +111,10 @@ struct _Delimiters {
    static let Space = UnicodeScalar(0x20)
    static let HorizontalTab = UnicodeScalar(0x09)
    static let Colon = UnicodeScalar(0x3a)
+    static let Backslash = UnicodeScalar(0x5c)!
+    static let Comma = UnicodeScalar(0x2c)!
+    static let Dquote = UnicodeScalar(0x22)!


Nit: Mind if we call this DoubleQuote? We already spell HTAB HorizontalTab instead of Htab, and it just reads a little more clearly, I think, in the context where we have comments referring to "dequoted" strings and then code that refers to "dquote".

Sure! Anything to reduce confusion. Change pushed.

lxbndr · 2020-09-28T16:40:04Z

Build failure on Linux looks unrelated. @MaxDesiatov Could re-run fix that, WDYT?

Basic implementation of authentication challenge parsing.

lxbndr · 2020-10-30T13:52:41Z

Rebased to latest main.

MaxDesiatov · 2020-10-31T18:13:54Z

@swift-ci please test

MaxDesiatov · 2020-12-02T09:59:07Z

Would anyone mind if this is merged?

MaxDesiatov · 2020-12-02T09:59:27Z

@swift-ci please test

spevans · 2020-12-02T11:24:25Z

@MaxDesiatov No I dont mind, lets get this in. I think have some URLSession fixes that rely on this so would be good to progress those.

MaxDesiatov · 2020-12-02T12:14:18Z

Linux failure seems to be unrelated

MaxDesiatov · 2020-12-03T12:12:39Z

@swift-ci please test Linux platform

spevans · 2020-12-03T16:49:52Z

@swift-ci please test Linux platform

spevans reviewed Apr 6, 2020

View reviewed changes

millenomi reviewed May 11, 2020

View reviewed changes

compnerd marked this pull request as draft May 12, 2020 23:15

lxbndr force-pushed the protection-space-realm branch from 923c868 to 507a169 Compare August 27, 2020 16:26

lxbndr marked this pull request as ready for review August 27, 2020 16:43

millenomi approved these changes Aug 27, 2020

View reviewed changes

lxbndr changed the base branch from master to main September 23, 2020 07:41

lxbndr force-pushed the protection-space-realm branch from 507a169 to 51938c9 Compare September 25, 2020 16:30

xwu reviewed Sep 25, 2020

View reviewed changes

lxbndr force-pushed the protection-space-realm branch from 51938c9 to 686675c Compare September 25, 2020 22:10

This comment has been minimized.

Sign in to view

Create URL Protection Space from HTTPURLResponse

ca2fe94

Basic implementation of authentication challenge parsing.

lxbndr force-pushed the protection-space-realm branch from 686675c to ca2fe94 Compare October 30, 2020 13:51

This comment has been minimized.

Sign in to view

spevans merged commit 6897b19 into swiftlang:main Dec 3, 2020

lxbndr deleted the protection-space-realm branch December 3, 2020 22:08

Create URL Protection Space from HTTPURLResponse #2761

Create URL Protection Space from HTTPURLResponse #2761

Uh oh!

Conversation

lxbndr commented Apr 5, 2020

Uh oh!

lxbndr commented Apr 5, 2020

Uh oh!

spevans commented Apr 5, 2020

Uh oh!

spevans commented Apr 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millenomi commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lxbndr commented May 11, 2020

Uh oh!

millenomi commented Aug 26, 2020

Uh oh!

lxbndr commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

millenomi commented Aug 26, 2020

Uh oh!

lxbndr commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lxbndr commented Aug 27, 2020

Uh oh!

millenomi commented Aug 27, 2020

Uh oh!

millenomi commented Aug 27, 2020

Uh oh!

lxbndr commented Sep 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

lxbndr commented Sep 28, 2020

Uh oh!

This comment has been minimized.

lxbndr commented Oct 30, 2020

Uh oh!

This comment has been minimized.

MaxDesiatov commented Oct 31, 2020

Uh oh!

MaxDesiatov commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spevans commented Dec 2, 2020

lxbndr commented Aug 26, 2020 •

edited

Loading

lxbndr commented Aug 27, 2020 •

edited

Loading

MaxDesiatov commented Dec 2, 2020 •

edited

Loading

MaxDesiatov commented Dec 2, 2020 •

edited

Loading