haussli / draft-dahm-opsawg-tacacs-security

IETF draft for new tacacs+ security features
1 stars 1 forks source link

Alan Dekok: Proxy concerns #51

Open dcmgashcisco opened 2 years ago

dcmgashcisco commented 2 years ago

Section 4 concerns me. This is not just adding TLS to TACACS+, it's adding an entirely new flow: TACACS+ proxying. This is a major change to TACACS+, and I would strongly suggest moving it to another document.

I would also recommend not defining a new packet type which extends the horrific TACACS+ packet format. We've come a long way in protocol design since the mid 1990s. The packet format is awkward, at best. It is difficult to create and/or parse correctly.

Plus, what if the proxy wishes to forward information about the client certificate, or server certificate which was used? The current format makes this impossible.

My recommendation would be to take a page from DHCPv6, and just add an encapsulation layer. e.g. a TACACS+ header with minor version 2, and a new type signifying "proxied packet". That packet could simply be a container for encapsulating the original packet.

i.e. instead of re-encoding the entire packet (with all of the issues and errors that entails), just take the original packet, add a proxy header, and send that essentially verbatim. This is much simpler, and has much less room for errors.

i.e. a proxied packet could look like:

TACACS+ header (minor = 2, type = proxied)
proxy header {
    length of proxy header
    flag for is next header a proxied packet.
    original src / dst ip / port
    ... potentially other data ..         
}
length of original packet
verbatim copy of original packet

That format is simpler to encode/decode, simpler to understand, and is easily extensible to add new fields. It also allows for multiple layers of proxying, which the current draft doesn't.

dcmgashcisco commented 2 years ago

Initial discussion points on this feedback

1: Moving it to another document: may be problematic, as Section 4 enables the packet size to support ssh (I believe.... we may find some other way to do this) so... it could require moving SSH support out as well,

2: Proxy use case is just a single example of a new attribute use case, We already support T+ proxy its is just limited, this arg just makes Proxy work less badly. I'm not quite sure what the multiple levels of proxying are about.

Actually, I suspect the whole proxy issue may be being misinterpreted. I suspect that it may be better to remove it as a distraction from the document.

3: using a new Packet format: I'm open to this. but I think I need to clarify the proxy issue first.

dcmgashcisco commented 2 years ago

Response to Alan:

The purpose of Section 4 is the alignment of arguments handling in Authentication phase of the T+ protocol with the authorization and accounting phases. To recap: authorization and accounting phases have extensible arguments handling, authentication does not. Section 4 brings the same patterns we have in authorization and accounting into authentication.

Just like the flexibility that arguments give authorization and accounting, adding this same pattern in authentication provides flexibility to support flows such as SSH key distribution.

The comment does highlight how the document confuses the technical description of the alignment, with the application of that alignment. We can see that we need to make this distinction clearer, so we will remove the proxy flow parts from section 4. Proxy will be discussed more below.

In terms of the protocol format: we favored consistency: using the same pattern for authentication for the argument-type data that was already defined in authorization and accounting we considered to be more helpful to implementors than introducing a new format (at least the current parsers could be reused).

The protocol style picked will always go stale and eventually become obsolete. If we picked a different format for the argument data for authentication, we will guarantee inconsistency with what is defined in other phases. Eventually, the new style we pick is itself likely to be obsoleted, in which case we will have both a obsoleted and inconsistent choice.

That said, if the group favors a modern approach for adding arguments to authentication, over consistency of what is already in the protocol, we will take this back and update the doc.

Regarding the Proxy Flow specifically: in the experience of the authors this is not a new flow for T+: it is an established practice in the field. However, there is a weak point for T+ in the proxy case: the authentication packets do not contain the original client. T+ servers tend to deduce the client from the other end of the TCP connection. This limitation means the final T+ server will only be able to deduce the proxy, and not the original client. But, policy normally needs to be evaluated according to what the original client is, NOT the proxy server. So, the intent here is merely to allow a flow that is already used in the field work better.

As discussed above, our first thought is to take the proxy out from section 4 and move it to a new section in the document. If it is still regarded by the group that adding this enhancement for proxy support is outside the allowable remit of the document, then we will not incorporate the proxy section at all.

td-tacacs commented 2 years ago

Hi Douglas,

the response looks good to me.

Thanks, Thorsten

haussli commented 2 years ago

It is perfect.