Re: [Ace] AD review of draft-ietf-ace-oauth-authz-24

[Benjamin Kaduk <[e-mail]>]

(This is still "in backwards order" (per
https://mailarchive.ietf.org/arch/msg/ace/qy9wQX04zkLS3n4BHS0GxKKi1XM)
albeit with a much-longer-than-planned delay...)

On Tue, Oct 15, 2019 at 04:07:48PM +0200, Ludwig Seitz wrote:
> Hello Ben,
> 
> thank you for your thorough review.
> 
> I have taken the liberty to add numbers to your comments in order to 
> refer to them in a easier way.
> 
> I have fixed 93 your 113 and there are 20 left where I am asking for 
> clarifications. These are:
> 
>   6.), 12.), 16.), 19.), 34.), 39.), 41.), 45.), 52.), 57.), 58.), 65.), 
> 66.), 68.), 76.), 78.), 79.), 88.), 99.), 111.)

Thank you for the numbering; that should help with keeping track of things.
I will try to trim the no-clarification-needed ones from this note.
> 
> Note that 39.) Requires input from OAuth experts. Hannes?
> 
> If you whish to inspect the changes made on other review issues, please 
> consult the commits here:
> https://github.com/ace-wg/ace-oauth/commits/master
> 
> 
> Detailed comments below.
> 
> /Ludwig
> 
> > 
> > Hi all,
> > 
> > The length of this review notwithstanding, this document is generally in
> > good shape -- there's a bunch of localized items to tighten up, and we
> > can flesh out the security considerations some more, but nothing too
> > drastic should be needed.
> > 
> > 1.)
> > Perhaps the most far-reaching changes needed
> > will be to rename the "profile" claim, since that has already been
> > allocated to OIDC Core for a very different usage.
> 
> [LS] FIXED. I renamed the "profile" claim and parameters to "ace_profile"
> Note that this will require changes in all of the profile drafts as well.

Indeed.  It would be great if someone (the chairs? Other volunteers are
surely welcome) could make a list of changes that affect the profile
documents, and make a pass over each in turn to find the affected areas.

> > 6.)
> > Section 1
> > 
> >    Authorization is the process for granting approval to an entity to
> >    access a resource [RFC4949].  The authorization task itself can best
> >    be described as granting access to a requesting client, for a
> >    resource hosted on a device, the resource server (RS).  This exchange
> > 
> > I had to pause for a while after reading this and ponder whether I
> > agreed with it.  I think that my reticence stems from some friction
> > between the most generic abstract definition of "resource" and a more
> > specific one used in the web/HTTP world and, to a lesser extent, the
> > world of URIs and URNs in general.  The resources we are discussing here
> > are not always specific named resources, but can also refer to
> > attributes or capabilities mediated by a RS; similarly, we may be
> > creating/modifying named resources as part of the resource access
> > performed by a client in the OAuth model.  I don't think it's wise to
> > diverge from the RFC 4949 definition just yet, but am still considering
> > whether adding some additional clarifying text here is worthwhile.
> 
> [LS] I would argue that this specification is applicable even to the wider
> definition of "resource" that you are thinking of. Since OAuth 2.0 leaves

Oh, I agree, and was not intending to say otherwise with my comments above.
Rather, I was considering that some [other] readers might see the word
"resource" and go straight to "web resource named by URI", and wondering if
we could reword slightly to avoid that.

> the definition of "scope" up to the specific applications, and the ACE
> framework does not change this, we can deal with both web/HTTP/CoAP 
> resources
> (named by URIs or URNs) and any other type of resources where the 
> application
> can map the resource in question to a set of scopes.
> I am therefore inclined to say that this section is fine, but I'd be glad to
> hear the result of your considerations on that matter.

I see three potential options so far:

(1) no change
(2) in the first sentence, s/resource/generic resource/
(3) add a new sentence as the third sentence, similar to "This resource
might be a Web or similar resource addressed by URI, but in general can be
a more generic or abstract resource provided by the RS".

I'm happy to advance the document with any of those three (and probably
with other versions, if any arise).

> > 12.)
> >       A refresh token in OAuth 2.0 is a string representing the
> >       authorization granted to the client by the resource owner.  The
> >       string is usually opaque to the client.  The token denotes an
> > 
> > Apparently I'd forgotten that this couldn't be binary.
> 
> [LS] From RFC 6749, section 1.5: "A refresh token is a string ..."
> No qualifiers such as "binary" so I interpret this to mean a text-string.
> Do you want us to add some clarification?

[this is handled in the other thread]

> > 16.)
> > Section 3.2
> > 
> >    One application of COSE is OSCORE [I-D.ietf-core-object-security],
> >    which provides end-to-end confidentiality, integrity and replay
> >    protection, and a secure binding between CoAP request and response
> >    messages.  In OSCORE, the CoAP messages are wrapped in COSE objects
> >    and sent using CoAP.
> > 
> >    This framework RECOMMENDS the use of CoAP as replacement for HTTP for
> >    use in constrained environments.
> > 
> > Do we have a reason to mention OSCORE if we're not going to make a
> > recommendation about its use?
> 
> [LS] We also mention DTLS and TLS without making any recommendation about
> which to use. I would suggest to either remove all of it or to add a 
> sentence
> noting that this is an enumeration of some security options, and the choice
> depends on the specific application scenario.

Adding a sentence feels like a slightly better option to me, though it
could easily go either way.

> > 19.)
> > Section 5
> > 
> >    Credential Provisioning
> >       For IoT, it cannot be assumed that the client and RS are part of a
> >       common key infrastructure, so the AS provisions credentials or
> >       associated information to allow mutual authentication.  These
> >       credentials need to be provided to the parties before or during
> >       the authentication protocol is executed, and may be re-used for
> >       subsequent token requests.
> > 
> > nit: either "before or during the execution of the authentication
> > protocol" or "before or during the authentication protocol's execution".
> > And just to double-check that we mean the authentication protocol of
> > provisioning in the last sentence, not the authorization protocol that
> > occurs between the client and RS.
> 
> [LS] The whole last sentence was a bit off. I would suggest:
> "The resulting security association between client and RS may then be 
> re-used
> by binding these credentials to additional access tokens." Does that
> sound better?

That definitely reads more coherently, yes.  It makes implicit the fact
that there is an authentication protocol that gets run, which is probably
okay, and I'm not sure whether "additional" is quite in line with the way
the previous sentence is formulated.  (Maybe it makes more sense in
context, which I don't have in front of me right now.)

> > 32.)
> > Section 5.6.1
> > 
> >    The client sends a POST request to the token endpoint at the AS.  The
> >    profile MUST specify how the communication is protected.  The content
> > 
> > In the previous section we said that maybe even other transports than
> > coaps or https would be possible; are we limited to those that have POST
> > verbs?
> > Also, a similar comment as above about what attributes the protection
> > entails seems to apply.
> 
> [LS] This will need a major rephrasing of the text.
> I see two options here:
> 
> 1.) We rewrite all parts to use a neutral language in general and specify
> POST/GET etc. for transports that have these verbs.
> 
> 2.) We state in the beginning that transports that do not use RESTful verbs
> should use the best equivalent.
> 
> Option 1. would get a bit cluncky, while option 2. might be a bit confusing
> Do you have a specific preference?

[this is covered in the other thread, I think]

> > 34.)
> >    o  A client MUST be able to use the parameters from
> >       [I-D.ietf-ace-oauth-params] in an access token request to the
> >       token endpoint and the AS MUST be able to process these additional
> >       parameters.
> > 
> > [we might get someone complaining that if support for these is mandatory
> > they might as well be in the core document.  I don't mind the split, but
> > be forewarned.]
> 
> [LS] The split came to pass since some parameters were expected to become
> obsolete with some ongoing OAuth work and we wanted to have a document that
> was independent from the core that could be overridden.
> I'm not sure this still applies, since the main culprit was the "resource"
> parameter defined in OAuth-resource-indicators. The remaining "req_cnf"
> parameter for requesting specific confirmation claims might become 
> obsolete if
> the OAuth WG takes up their PoP work again.

Thanks for the extra background here; it seems like we should leave it
as-is for now.

> > 37.)
> > 
> >    Figure 7 shows a request for a token where a previously communicated
> >    proof-of-possession key is only referenced.  Note that the client
> >    performs a password based authentication in this example by
> >    submitting its client_secret (see Section 2.3.1 of [RFC6749]).  Note
> > 
> > RFC 6749 says about the bare "client_secret" that "[i]ncluding the
> > client credentials in the request-body using the two parameters is NOT
> > RECOMMENDED and SHOULD be limited to clients unable to directly utilize
> > the HTTP Basic authentication scheme (or other password-based HTTP
> > authentication schemes)."  I know this is "just an example", so our
> > obligations are somewhat weak, but I still think we should note that
> > this is not recommended by 6749 (but since there is no CoAP analogue of
> > HTTP Basic, it's the only plain-password scheme available).
> 
> [LS] My take is to modify this example and remove the client_secret part.
> It feels wrong to use NOT RECOMMENDED stuff, even if it only is an example.

Thanks!

> > 39.)
> >    Refresh tokens are typically not stored as securely as proof-of-
> >    possession keys in requesting clients.  Proof-of-possession based
> >    refresh token requests MUST NOT request different proof-of-possession
> >    keys or different audiences in token requests.  Refresh token
> >    requests can only use to request access tokens bound to the same
> >    proof-of-possession key and the same audience as access tokens issued
> >    in the initial token request.
> > 
> > This is perhaps something of a philosophical question, but if a refresh
> > token is only usable at the token endpoint, in some sense its audience
> > is definitionally the AS.  So there's a little bit of a mismatch in
> > treating it as having the audience value that the access tokens issued
> > from it will have.  I don't know the background for audience-restriced
> > refresh tokens in regular OAuth 2.0, though, so hopefully someone can
> > educate me.
> 
> [LS] I'm equally confused. I suggest that Hannes or one of the other OAuth
> experts give us a hint on that one.

[We had some stab at this in the other thread, but additional input might
still be in order]

> > 40.)
> > Section 5.6.2
> > 
> >    Note that the AS decides which token type and profile to use when
> >    issuing a successful response.  It is assumed that the AS has prior
> > 
> > Would it be useful to note that this can be shaped by the "req_cnf"
> > contents?
> 
> [LS] I don't think token type would be affected by the requested 
> confirmation
> key format. The selected profile however may well be. A note to this 
> effect has
> been added.

I may have been thinking that if "req_cnf" was absent, then we might not
get a pop token at all.  Thanks for thinking about this some more; I trust
your conclusion.

> > 
> > 41.)
> >    token_type:
> >       This parameter is OPTIONAL, as opposed to 'required' in [RFC6749].
> >       By default implementations of this framework SHOULD assume that
> >       the token_type is "pop".  If a specific use case requires another
> >       token_type (e.g., "Bearer") to be used then this parameter is
> >       REQUIRED.
> > 
> > When we are "weakening" the formal requirements of the parent spec like
> > this, we should be careful about what happens when someone is trying to
> > use the ACE stuff but ends up needing to go over HTTPS like traditional
> > OAuth -- do they have to fall back to the OAuth required behavior in
> > that case or are we trying to preempt that as well?
> 
> [LS] The use case here is a constrained connection between client and AS
> (e.g. due to a constrained client), where we try to save the bytes for
> sending a token_type by defining a default. I would guess that a HTTPS based
> connection would not have such constraints and would therefore be able 
> to send
> the token_type = "pop" as required by RFC6749. Should we explain this 
> somewhere
> and describe the cases where RFC6749 behaviour is warranted?

[this is covered in the other thread]

> > 45.)
> >    o  A response code equivalent to the CoAP code 4.00 (Bad Request)
> >       MUST be used for all error responses, except for invalid_client
> >       where a response code equivalent to the CoAP code 4.01
> >       (Unauthorized) MAY be used under the same conditions as specified
> >       in Section 5.2 of [RFC6749].
> > 
> > 6749 has a case where 401 MUST be used, but I think it does not apply to
> > us (since WWW-Authenticate would have to have been used).  I'm not sure
> > whether it makes sense to note that or not, though.
> 
> [LS] The 401 in 6749 is the invalid_client case. For CoAP a 4.01 is also
> allowed (MAY) instead of 4.00 if this is warranted. I do not believe further
> action is warranted at this point. Do you agree?

Yes.  (Thank you for thinking about it!)

> > 52.)
> > Section 5.7.4
> > 
> > "scope" is showing up with the key 9 a lot; I have mixed feelings about
> > having the CBOR map key value be aliased across different contexts (but
> > it's probably too late to change without disruption anyway).
> 
> [LS] Scope is used in 4 places:
> 
> I. AS Request Creation Hints
> II. /token request and response parameter
> III. CWT claim
> IV. /introspection response parameter
> 
> I claim that in all 4 cases we expect the same content with the same
> formatting and semantics:  I. tells the client what to put into II.
> which in turn informs the AS about the requested claim value in III.
> and IV. is supposed to reflect that claim value. I therefore don't feel
> hesitant to use the same abbreviation for all four cases.
> Is that acceptable or do you whish us to perform any action on this?

[we covered this in the other thread]

> > 53.)
> > Section 5.8
> > 
> >    In order to facilitate offline processing of access tokens, this
> >    document uses the "cnf" claim from
> >    [I-D.ietf-ace-cwt-proof-of-possession] and specifies the "scope"
> >    claim for JWT- and CWT-encoded tokens.
> > 
> > [looks like draft-ietf-oauth-token-exchange is going to beat us for the
> > JWT version of "scope":
> > https://www.iana.org/assignments/jwt/jwt.xhtml#claims ]
> 
> [LS] Agree, I will change the whole document to refer to this registration
> instead. Life punishes those who come too late.

And those reliant on people who come too late, mea culpa.

> > 57.)

Whoops, trimmed to fast, but that's a fine explanation; thanks.

> > 58.)
> >    Profiles MUST specify whether the authz-info endpoint is protected,
> >    including whether error responses from this endpoint are protected.
> >    Note that since the token contains information that allow the client
> >    and the RS to establish a security context in the first place, mutual
> >    authentication may not be possible at this point.
> > 
> > We'll need some careful reasoning about this for the security
> > considerations, since the authz-info transaction can impact what profile
> > the RS thinks is in use.  E.g., whether a network attacker could
> > cause the client to think that a different (vulnerable) profile is in
> > use than the one the RS expects to use.
> 
> [LS] Noted. Do you think the reasoning in section 6.5 needs to be extended?

I think we should add some more text, yes.
Specifically, we should mention that the authz-info interaction can affect
what profile RS will use (e.g., via "ace_profile"), and that profile
developers should be conscious of the risk of downgrade attacks that
involve other profile types.  (Am I reading this right that the client will
know what profile to use by the time it is ready to post to the authz-info
endpoint and that the response will not change what profile the client
uses?  Specifically, even if a client supports multiple profiles that use
different methods for token transport, a client is not going to try one
method/profile and then fall back to a different one if the first one
(transiently) fails?)

> > 65.)
> >       specification defines the following approach: The claim "exi"
> >       ("expires in") can be used, to provide the RS with the lifetime of
> >       the token in seconds from the time the RS first receives the
> >       token.  This approach is of course vulnerable to malicious clients
> >       holding back tokens they do not want to expire.  Such an attack
> > 
> > It also has suboptimal behavior if the RS loses state (e.g., by
> > rebooting), and possibly requires the RS to store indefinitely all
> > tokens with an "exi" value.  I have mixed feelings about specifying it
> > at all, though I concede it probably does have some value.  Regardless,
> > I think a dedicated subsection in the security considerations is in
> > order.
> 
> [LS] We wanted to provide some solution for expiring tokens for RSes 
> that have
> no connectivity and no synchronized clocks. Using the "exp" claim in 
> such cases
> would have pretty unpredictable results.
> I have extended section 6.3 in the security considerations to go into 
> the detail
> of "exi", please have a look if this covers the necessary issues.

I think we should also say something about the amount of such persistent
storage potentially growing without bound, as those counters (or some
similar indication) are the only thing that will cause the RS to reject
tokens that have been used the requisite number of times.  So, RS state
requirements grows with the number of 'exi'-bearing tokens that are issued
for them.  I suppose a bloom filter might be a way out, though...

> > 66.)
> > Section 5.8.4
> > 
> >    The AS provides the client with key material that the RS uses.  This
> >    can either be a common symmetric pop-key, or an asymmetric key used
> >    by the RS to authenticate towards the client.  Since there is no
> > 
> > Can you walk me through how a symmetric PoP key would be used for mutual
> > authentication (i.e., authentication of RS to client)?  It's easy/common
> > to do the PoP the other way, making the client prove it knows the key
> > associated with the token, but the other direction is not always
> > possible, depending on the protocol.
> 
> [LS] The reasoning is that if both client and RS perform a 
> proof-of-possession of
> the symmetric PoP key to each other, they can be considered to be mutually
> authenticated, under the provision that no other parties (except for the 
> AS) have
> access to said key.  My understanding is that e.g. DTLS-PSK provides 
> this kind of
> guarantee. Do you want us to clarify this reasoning in the document?

Ah, right, of course.  I must have been stuck in some
in-band-token-delivery mindset when reading this and was thinking too
narrowly.

> > 68.)
> >    o  The client performs an introspection of the token.  Although this
> >       is not explicitly forbidden, how exactly a client does
> >       introspection is not currently specified for OAuth.
> > 
> > I'm pretty sure this is overtaken by events (sorry for my part in
> > that!).  E.g., draft-ietf-oauth-jwt-introspection-response discusses
> > clients doing introspection, and even RFC 7662 itself discusses using a
> > client secret to authenticate to the introspection endpoint.  I think
> > there's another document between those two that's also relevant, but
> > can't find it right now
> 
> [LS] I am not so sure. When reading the fine print in both RFC 7662 and
> draft-ietf-oauth-jwt-introspection-response, I find that when they mention
> the term "client", they refer to the protected resource / RS as being a 
> client of
> the AS introspection endpoint.  A client holding an access token and 
> performing introspection is never explicitly mentioned in both 
> documents, to my best
> knowledge.

It seems like we should try to check on this in Singapore, while the usual
suspects are easily at hand.

> > 69.)
> > Section 6
> > 
> > There could perhaps be some security considerations relating to
> > discovery of RSes/resources thereon, but since we don't really talk
> > about that much to begin with, it's probably okay to skip the security
> > considerations discussion as well.
> 
> [LS] Noted. I opt to skip this for the time being. If it comes up in the 
> IESG
> review we can revisit this issue.

Okay.

> > 72.)
> >    A large range of threats can be mitigated by protecting the contents
> >    of the access token by using a digital signature or a keyed message
> >    digest (MAC) or an Authenticated Encryption with Associated Data
> > 
> > I won't be surprised if someone asks for examples of that "large range
> > of threats", so be prepared to answer...
> 
> [LS] Noted. I take it that no action is required here at this point.

Right.

> > 76.)
> >    It is important for the authorization server to include the identity
> >    of the intended recipient (the audience), typically a single resource
> >    server (or a list of resource servers), in the token.  Using a single
> >    shared secret with multiple resource servers to simplify key
> >    management is NOT RECOMMENDED since the benefit from using the proof-
> >    of-possession concept is significantly reduced.
> > 
> > I think we should word this more clearly with respect to "single shared
> > secret" -- is this the credential used by the RS to authenticate itself
> > to the client?  I'm not even sure if this text is intended to be
> > describing a workflow using symmetric or asymmetric keys.
> 
> [LS] I added some clarification. Can you double-check that this makes
>   more sense?

It does, thank you.
(Though rereading it does make me wonder whether anyone would be foolish
enough to use the same symmetric PoP secret for multiple clients with the
same RS...)

> > 78.)
> > Section 6.1
> > 
> > I think we should have a little bit more discussion about what attacks
> > are possible even when a client hard-codes a list of trustworthy ASes,
> > e.g., when a device in one AS's purview is compromised and tries to get
> > the client to use a different (possibly also compromised, or maybe just
> > buggy) AS than the one that's supposed to be responsible for the device
> > in question.  In short, yes, spoofing is only possible within that set
> > of trusted ASes, but spoofing can still cause problems.
> 
> [LS] I have added some text in section 6.4 Please have a look if this
> covers what you were aiming at.

That's pretty good, thanks!
I'd prefer to also have (in the second sentence of the second paragraph) a
mention about what an AS (in the hard-coded list) would do when receiving
the incorrect request from the misdirected client, though.
(Also, nit, s/ redentials/ credentials/)

> > 79.)
> > Are there any AS parameters other than URI that might be useful for an
> > out-of-band-configured list of valid values?
> 
> [LS] One might want to include the public key or certificate of the AS.
> Do you want us to expand this section to include such parameters?

I'd consider adding another sentence like "Information used to authenticate
the AS, such as a public key or certificate fingerprint, might be
provisioned alongside an AS's URI, depending on the deployment scenario".
What do you think?

> > 88.)
> > Section 8.3
> > 
> >    Name  The OAuth Error Code name, refers to the name in Section 5.2.
> >       of [RFC6749], e.g., "invalid_request".
> > 
> > We should refer to the OAuth registry as the authority on names, not the
> > immutable RFC.  (Similarly for the other mappings registries; I won't
> > repeat it each time, though for the later ones we're already doing the
> > right thing.)
> 
> [LS] This is interesting. The referenced section (5.2 of RFC6749) is not
> mirrored in any IANA registry. I have put the question to the OAuth WG.

IIRC, you filed https://www.rfc-editor.org/errata/eid5873 for this and I
submitted a registration request at
https://mailarchive.ietf.org/arch/msg/oauth-ext-review/4WaM6n6JetFsLI6_T9S8wNVqP74
.  It's been more than two weeks, so I should follow up there...

> > 97.)
> > Section 10.1
> > 
> > We may get some debate about whether IANA registries are properly
> > Normative of Informative references, but we can wait for that to happen
> > -- no need to do anything now.
> 
> [LS] Noted. Does the AD have a position on this?

I think there's a reasonable argument for keeping them as normative.  We
probably see them as informative more often (in general), though, probably
because people are concerned about having downrefs to something that's not
a standards-track RFC (which would happen due to a misunderstanding of the
rules surrounding downrefs, IMO).

> > 98.)
> > Section 10.2
> > 
> > If we're using RFC 4949 for terminology definitions, I think that makes
> > it a normative reference.
> > 
> > If we REQUIRE CBOR when used with CoAP, that also feels like a normative
> > reference.
> > 
> > I also think 7519 needs to be normative, since we mandate some of its
> > processing rules.
> 
> [LS] RFC 4949 is informational, so it cannot be normative. I would argue
> that we are just using it to clarify the meaning of our terminology.

It's okay to have a normative reference to an informational document; we
just need to call it out in the IETF LC announcement (which the
tooling/secretariat should take care of "automagically").
Furthermore, RFC 4949 is already listed at
https://datatracker.ietf.org/doc/downref/ as an "acceptable downref", so we
don't even have to do that, in this case.

> I agree with the other two and have FIXED that.
> 
> 
> > 99.)
> > Appendix A
> > 
> >    Proof-of-Possession:
> > 
> >       This framework makes use of proof-of-possession tokens, using the
> >       "cnf" claim [I-D.ietf-ace-cwt-proof-of-possession].  A
> >       semantically and syntactically identical request and response
> >       parameter is defined for the token endpoint, to allow requesting
> >       and stating confirmation keys.  This aims at making token theft
> > 
> > nit: the wording here is a little weird in terms of specifying what is
> > identical.  I'm not sure I have a great suggestion for improvement,
> > though; what I'm coming up with is a lot more words.
> 
> [LS] How about:
> A request parameter "cnf" and a Response parameter "cnf", both having a
> value space semantically and syntactically identical to the "cnf"
> claim, are defined for the token endpoint, to allow requesting and
> stating confirmation keys.

That ... works quite well.  Thank you!

> > 111.)
> >       an access token as an opaque string, which it can match to the
> >       specific client, a targeted audience and a symmetric key.  The
> > 
> > So we're handing out a symmetric PoP key for a long-lived token that may
> > be used at multiple resources?  Isn't that the example we gave earlier
> > that lets RSes impersonate the client to each other?
> 
> [LS] Since the doors (RSs) are all part of the same system we assume 
> that this is not a issue here.  I added some explanatory text to clarify 
> these
> assumptions. Do you deem this acceptable?

It's good enough for me, yes (and thank you).  I can't guarantee that Roman
won't want more changes, though :)

> 
> Side note: If we used asymmetric pop keys instead, we would either have 
> to provision all the doors' keys to the client up-front, or define a whole
> new protocol (which we initially tried with the "clientToken" concept, 
> but later dropped) that provides the client with the RS's public key 
> during the
> token POST or access attempt.

That does sound unpleasant...


If you want to get a new revision up to make these last few changes during
the blackout period, I'm happy to approve a manual posting by the
secretariat.  (OTOH, since IETF LCs that overlap with the meeting week get
extended automatically, it wouldn't necessarily get the document on an IESG
telechat any sooner.)

Thanks for all the updates and detailed discussion!

-Ben