Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alan Dekok: Proxy concerns #51

Open
dcmgashcisco opened this issue Apr 14, 2022 · 4 comments
Open

Alan Dekok: Proxy concerns #51

dcmgashcisco opened this issue Apr 14, 2022 · 4 comments
Assignees

Comments

@dcmgashcisco
Copy link
Collaborator

dcmgashcisco commented Apr 14, 2022

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 dcmgashcisco self-assigned this Apr 14, 2022
@dcmgashcisco
Copy link
Collaborator Author

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
Copy link
Collaborator Author

dcmgashcisco commented Apr 18, 2022

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
Copy link
Collaborator

Hi Douglas,

the response looks good to me.

Thanks,
Thorsten

@haussli
Copy link
Owner

haussli commented Apr 28, 2022

It is perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants