[secdir] draft-ietf-cdni-edge-control-metadata-12 ietf last call Secdir review

[Mike Ounsworth via Datatracker <[e-mail]>]

Document: draft-ietf-cdni-edge-control-metadata
Title: Content Delivery Network Interconnection (CDNI) Edge Control Metadata
Reviewer: Mike Ounsworth
Review result: Has Issues

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG.  These comments were written primarily for the benefit of the
security area directors.  Document editors and WG chairs should treat
these comments just like any other last call comments.

The summary of the review is HAS ISSUES

This draft is essentially defining a file-based config for CORS.

Every time I've worked with CORS on a web server, it's either simple -- ie
static CORS headers across the whole web server -- or it requires like actual
server code to set all the CORS settings in a cohesive way, and then still the
security team find problems because CORS is so complicated and hard to get
right on the first pass. I have never personally seen a config file that's able
to capture the full complexity of a dynamic CORS configuration. Do those exist?
If so, that would be useful as design input for this draft, maybe as an
informative reference.

I know that the intention here is to simply forward an existing CORS config,
not to get into the details of CORS, however, I think there are valid CORS
configurations that cannot be expressed in the current MI model, and the MI
model is missing information that the dCDN needs in order to properly set CORS
headers in some edge cases -- leading to either security issues at the UA or
strange behaviours as the dCDN tries to simultaneously comply with the MI
config and the WHATWG FETCH spec. I suggest that more work is needed to align
this document with the CORS definitions in the WHATWG FETCH spec. I think the
definitions of the new MI properties needs way more security consideration text
documenting the complex interactions between the specified config settings and
the security issues that can arise from "strange" configurations. There also a
number of unimplementable or undefined behaviours that arise from
inconsistencies between this document and its normative references; fortunately
I think those are easily fixed.

Issues
------

Presented in what I feel is most-important-first order.

1.
This is a meta-comment that applies to all the others.
Given that this draft is, almost in its entirety, dealing with configuring a
security mechanism (a rather complex and difficult-to-configure one at that),
there needs to be much more Security Consideration discussion on implications
of misconfiguration and whether the way CORS config is being represented in the
MI is a tight fit with the definition in the WHATWG FETCH spec, or whether
there are gaps that lead to security implications.

I'm not really qualified to come up with the list, but, as an example, I could
imagine that propagation delay of the MI could induce security issues in some
contexts, for example if the uCDN adds a new resource before its corresponding
CORS policy has propagated to the dCDN. You want these things to fail closed
with either the dCDN or the UA refusing to serve the content to the requesting
app until the correct config catches up. I am not qualified to suggest how
exactly you achieve that within this tech stack.

2.
As a general point across the whole draft: the potential for the dCDN to reject
the request (return a 403) based on CORS headers is never discussed. In
general, it's a totally valid use of CORS to use it to enforce that your REST
API and static resources will only serve javascript clients loaded from your
domains, and will reject requests from javascript clients loaded from any other
domain. I myself have built web apps that use CORS in this way.

This seems like a serious omission from the document to either provide a
mechanism for configuring this, or to state clearly that it's out-of-scope for
the CDN use case.

I suspect that accommodating this will require a bunch of adjustments to your
MI model. On the other hand, if you are assuming that the correct behaviour is
to always serve the requested content down to the UA, and in cases where the
origin is wrong then the CORS headers will cause the UA to refuse to let the
non-authorized javascript access it, then that needs to be explicitly stated. I
don't think there's a security issue here with serving the "blocked content" to
the UA since CORS only applies to browsers and that content could easily be
fetched, for example, via curl, but some operators may want the ability to
configure the dCDN not to stream large content to a UA with a CORS header on it
that requires the UA to delete the content immediately. Serving a 403 in those
cases would often makes more sense for bandwidth use. Also, if refusal to serve
(403) is a valid outcome, then the Origin: header needs to be included in the
Vary: header, therefore this interacts with #3.

3.
HTTP Vary: header
I expected some discussion of how the mechanisms defined in this doc interact
with the HTTP Vary: header because as soon as you start playing with CORS, you
need to be careful that all CORS request headers that could possibly affect the
response are captured in the Vary: header. Failure to do this properly leads to
downstream proxies (and UAs?) thinking that they are free to cache this
response and serve it to any client in the future who requests it. As I
understand this draft, either the MI would need to carry Vary: info, or the
dCDN needs to decide for itself which CORS headers need to be in Vary:
dynamically based on the CORS payloads in the MI and then merge with with the
Vary: headers obtained from other sources (both of which sound fairly
complicated, and may itself require specification so that dCDNs don't screw it
up). I suggest that some of the text from:
https://fetch.spec.whatwg.org/#cors-protocol-and-http-caches needs to be lifted
into this draft, and then discuss the implications for the CDNI MI.

Fix: this is just an off-the-cuff idea that will need more thought, but maybe
every CORS MI property needs a sub-property "include-in-vary: Boolean, default:
true", with clear guidance about how the dCDN is supposed to merge this with
Vary: contents coming from other sources (append to existing Vary: or add a
second Vary: header -- I think RFC 7230 says that multiple Vary headers are
allowed, but this would need a closer look). Also would need a discussion of
the effects on the behaviour and security of dowstream proxies and UAs when
this is set to "false".

4.
wildcard-return
I feel that additional Security Considerations are needed, but I'm not 100%
sure what. My understanding is that:
    Access-Control-Allow-Origin: *
signals to the UA that this response is effectively public and can be shared
with scripts running on any origin. Under that understanding, the sample
configuration in Figure 1 seems odd to me because it's telling the dCDN: "If
you get a request with Origin: https://sourcepage.example.com, then respond
telling the UA that this resource is public and javascript from any Origin can
request it in the future". In other words, "Only https://sourcepage.example.com
can request this resource, but once they do, then the browser can share it with
javascript from any origin". I am struggling to think of a situation where
would be is desired behaviour. I suspect that for the vast majority of cases,
the thing you've shown in Figure 1 is actually a misconfiguration and a
security issue. So it seems odd (and maybe dangerous?) to have this as the very
first and most basic example in the draft. (I would love to be wrong here. If
there is an obvious use for this, then I would love to learn about it.)

In general, I find that the way the wildcard-return MI payload is defined, this
seems like a footgun in that operators who don't really understand CORS will
set it to "true" because it seems to make things work, but in fact it has
effectively undermined the CORS security mechanism. I suggest that the authors
should add text indicating what the intended use of the wildcard-return setting
is -- ie in what situations is it advisable and safe to set it to "true", and
what are the security implications of doing so.

This interacts with #3 because "Vary: Origin" can protect you here by telling
the UA that this response is only valid for future requests with exactly the
same Origin as the original request (effectively cancelling out the wildcard).
Had your document specified the interaction between
"Access-Control-Allow-Origin:" and Vary:, then possible I wouldn't have even
pointed this out.

Moreover (and I'm not a deep expert here), but I notice that this seems related:
https://fetch.spec.whatwg.org/#cors-protocol-and-http-caches
(particularly the last paragraph) which is saying that if *any* of the dCDN's
responses include "Access-Control-Allow-Origin: *" (or any other static
Origin), then *all* responses (CORS and non-CORS) are required to always
include an Access-Control-Allow-Origin: header and never a Vary: header. I do
not see any of this complexity discussed in the draft.

So, likely, the configuration shown in Figure 1 is hazardous in that it will
cause a dCDN to behave in a completely bizarre way in order to stay within the
guidance of the FETCH spec.

Also, FETCH clearly states that Access-Control-Allow-Origin cannot be * on a
credentialed request (cookies, TLS client certificates and HTTP
authentication). So correct dCDN handling of MI.wildcard-return requires a
fairly complicated interaction with MI.allow-credentials, which is not
currently discussed either.

5.
Section 3.3:
"
Property: no-origin-response-headers
"
I suspect you meant this section to allow a uCDN to direct a dCDN to set *CORS*
headers on responses. But as written, this allows a uCDN to direct a dCDN to
inject *any* header into responses. Depending on the trust relationship between
the dCDN and the uCDN, the dCDN may or may not be ok with this, and may
consider this a security vulnerability. Also, this feels like a footgun for
misconfiguration; for example when happens if you specify here to overwrite
some header that already exists in the response, like Content-Length or
Content-Type?

Fix: I think you either want to hard-code in the draft, or allow the dCDN to
configure, a list of headers that it will allow to be set by this MI property.
Probably what you meant is that only CORS headers can be listed here, so
listing those out is probably the intention.

This property probably also carries interactions with the HTTP Vary: header
that should be thought about.

Finally, this applies when the request contains no Origin: header.

5a.
Section 3.2 specifies "if the Origin request header does not match, the CORS
response headers MUST NOT be included in the dCDN response". I'm wondering out
loud here if returning no CORS response headers is always the right behaviour?
To my knowledge, WHATWG FETCH is silent on this, so there is no default
behaviour or guidance -- therefore servers are allowed to return CORS headers
in response to a failed Origin check. So would some implementers want the
ability to configure headers to return in this case -- ie add a
rejected-origin-response-headers that bahaves similarly to your
no-origin-response-headers?

6.
allow-list

6a.
path vs whole URI
This one is a "I know what you meant", but as written, I think it's
unimplementable, and the examples are technically invalid data.

Your examples of allow-lists in Figures 1 and 2 definitely look reasonable:

      "allow-list": [
        {
          "pattern": "https://sourcepage.example.com"
        }
      ]

      "allow-list": [
        {
          "pattern": "*://sourcepage.example.com"
        }
      ]

but those don't match the definition of allow-list in section 3.3.1, says:
"
Property: allow-list
Type: Array of Strings, where each String is a pattern as defined in Section
4.1.5 [RFC8006] " First, "allow-list": [{"pattern":
"https://sourcepage.example.com"}] is not an Array of Strings. It's an array of
objects each containing a "pattern" element containing a String. An Array of
Strings would be "allow-list": ["https://sourcepage.example.com",].

Second, and more importantly, Section 4.1.5 of [RFC8006] is path-absolute from
[RFC3986] and a path-absolute is a URI *path component*, starting with a "/",
and not a full URI and therefore clearly can't hold the data you show in
Figures 1 and 2. I don't see any datatype in Section 4 of RFC 8006 that can
properly hold a CORS Origin of the form scheme::host[:port] because none of
them include a Scheme, which is super important to CORS security since in CORS
"http://example.com" is a different Origin and MUST NOT match
"https://example.com".

This bug leads me to wonder about the level of implementation experience that
this draft has, because I think the samples in Figure 1 and 2 should be
rejected by a compliant parser.

Fix: 1) either change "Array of Strings" to match what's in the samples, or
change the samples to be Arrays of Strings. 2) I suggest that you drop the
reference to RFC8006 and cite the WHATWG FETCH spec's definition of Origin
directly. Alternatively, you may need to define a new datatype for representing
Origin: scheme://host[:port].

6b.
case sensitivity of origin string matching
The WHATWG Fetch spec, normatively referenced by this draft, constrains
the Origin header to lower case and requires byte-exact comparison --
Section 3.2 (https://fetch.spec.whatwg.org/#origin-header)

However, RFC 8006 specifies the matching rules as:
"
Property: case-sensitive
Mandatory-to-Specify: No.  Default is case-insensitive match
"
Since this draft does not override the default, an implementation gets a
case-insensitive match.
In practice, maybe not a real problem since Fetch guarantees Origin is always
lower case. So a conforming UA always emits lower case; this draft over-accepts
only from clients that are not implementing CORS correctly.

Fix: in order to fix 6a, you'll probably need to define a new JSON object
anyway, so for its syntax, I would copy verbatim the Origin matching rules from
Section 3.2 of the Fetch standard.

7.
Section 3.3 says:
"
Property: allow-credentials

    Description: The value the dCDN will include in the
    Access-Control-Allow-Credentials response header to a CORS preflight
    request.
"
but https://fetch.spec.whatwg.org/#http-responses says that
Access-Control-Allow-Credentials can be on a response to any CORS request, not
only preflights. I suspect this is an easy-to-fix typo.

8.
Section 3.3 says:
"
 Property: expose-headers
▎
▎ - Description: A list of header names the dCDN will include in the
▎   Access-Control-Expose-Headers response header of a CORS preflight
▎   request.
"
But, to my reading, https://fetch.spec.whatwg.org/#http-responses says clearly
that Access-Control-Expose-Headers is only allowed on non-preflight requests. I
think 3.3 has it backwards. I suspect this is an easy-to-fix typo.

9.
Section 3.1 -- the paragraph defining CORS says "HTTP client application", but
in fact CORS is specific to browser-type user agents (UA) and does not have any
security value for curl-type HTTP clients. The document should probably draw
the reader's attention to the fact that CORS' security model specifically
protects against malicious javascript running in an honest browser (UA) since
the UA is the CORS enforcement point (which is different than most web security
models where the server is the enforcement point). Keeping that front-of-mind
is important for reading the sections that follow.

10.
Section 3.3:
"
Property: allow-credentials"
Type: Boolean
"
WHATWG FETCH says that the only allowed value for
Access-Control-Allow-Credentials is lower-case "true". Declaring this as a
Boolean allows the value "false", which will result in undefined behaviour.

12.
The term "synthetic response" appears 4 times in the document, for example,
Section 3.2 uses the term "synthetic response" in the context of the dCDN
responding to an OPTIONS request. But I (and Claude) couldn't find the term
"synthetic response" defined anywhere in this document or in its references, so
I'm flagging "synthetic response" as an undefined term that could lead to
interpretation ambiguity. To my reading, it could mean one of two things: A)
this is a CORS thing where a "synthetic response" means a response to an
OPTIONS request which is a "synthetic" version of the response to the
corresponding GET / POST, or B) this is a CDN thing where a dCDN generates a
"synthetic response" any time that it responds without consulting the uCDN
(Claude thinks this is the intended meaning).

11.
"Using this GenericMetadata object permits the uCDN to: :"
Typo, double ":"
More importantly, are you sure this is "uCDN"? The list that follows would make
more sense if that was "dCDN", or maybe "allows the uCDN to set policy for the
following dCDN behavors"? That would also address my comment above about
refusal to serve the content as a valid outcome, because I think that's what
the second bullet is getting at: " * Match the value of the Origin request
header with a list of valid values. If it is successful, the dCDN will inject
the appropriate CORS response headers. ~ Though, for clarity, that bullet
should maybe also have a "If unsuccessful, ..." sentence.

Nits
----

"To permit shared cross-origin network requests, the CORS protocol defines a
set of request and response headers that must be present to permit access to
these restricted resources."

That lower-case "must" is a bit distracting since my brain tried to pick apart
whether that was normative or not. I suggest a re-word: "To permit shared
cross-origin network requests, the CORS protocol defines a set of request and
response headers that serve to allow servers and UA clients to selectively
relax the Same-Origin-Policy and grant access to these restricted resources."

"CDNs can include logic to dynamically generate the required response headers
based on the UA request without contacting the CP servers." I think the word
"can" is unnecessary in this sentence.

Section 3.2:
"
If none of those parameters are set in the configuration, the dCDN MUST forward
every CORS preflight request (OPTIONS method) to the uCDN servers " except
except DoS flooding protection, blah blah. May or may not be worth mentioning.