haussli / draft-dahm-opsawg-tacacs-tls13 Goto Github PK

Importing from haussli/draft-dahm-opsawg-tacacs-security#51

[dcmgashcisco] commented [on Apr 14]
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:

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.

Comment from Alan:

I don't think it's necessary for the first paragraph to re-explain how TACAC+ connections work.

Perhaps also explain that TACACS+TLS is little more than a TLS wrapper around un-obfuscated TLS. i.e. could it be implemented simply with stunnel + a historic TLS client/server?

Comment from Alan:

I would suggest some modifications to the terms. Perhaps:

 Historic TACACS+: as defined in RFC 8907

The intention here is to say also that it's not just "TACACS+ without TLS", but it's historic and should not be used.

TACACS+TLS: TLS transport with TACACS+ as the application data

Perhaps "TACACS+ over TLS" instead of "TLS for TACACS+" ?

Importing from haussli/draft-dahm-opsawg-tacacs-security#50

[dcmgashcisco] commented [on Apr 14]

It would be good to mention that TLS Server Name Indication (SNI) SHOULD be supported (https://datatracker.ietf.org/doc/html/rfc6066#section-3). That way one server can act as the TACACS+ host for multiple domains, and switch between them using SNI.

Reviewers proposed to evaluate RFC9325 as a posisble reference for TLS 1.3 recommendations.

ISSUES THAT CAUSED CONFUSION

3.2 Based on some questions from a developer in my team, it may be beneficial to explicitly state that earlier versions of TLS MUST NOT be supported. I think the text implies it but explicitly stating it may stop someone also adding support for earlier versions.
TBD

3.2.1 Cipher Requirements: "The cipher suites offered or accepted SHOULD be configurable so that operators can adapt." It is slightly unclear what the operators are adapting. Is this meant to be "The cipher suites offered or accepted SHOULD be configurable by operators" or is there a subtlety I'm missing?

FYI: Response from Alan: {{{

The implementation should expose enough TLS configuration that the administrator can change the cipher list if necessary. For example: https://github.com/FreeRADIUS/freeradius-server/blob/release_3_2_3/raddb/mods-available/eap#L407

That just takes configuration:

    cipher_list = "..."

and hands the string over to OpenSSL, which does it's magic. This lets the admin use "DEFAULT" in most cases. But if the admin needs to change that, it's simple to do so. }}}
TBD

3.3 TLS identification: There does not appear to be any network location based validation methods in section 5.2 of RFC5425. Is it meant to be a reference to the using a wildcard with a domain?

FYI: Response from Alan: {{{
I would agree that implementations should support IP-layer filtering by network. The TACACS+ server may be accessible on a management network, but perhaps only part of that network contains edge devices which will connect via TACACS+.

As a result, it is useful to have a configuration which can say "allow network FOO, forbid network BAR". This would be in addition to any TLS layer filtering.

}}}
TBD

6.1 TLS Use: "Unsecure Connections would be better served by separate Servers from the TLS Servers." It is not immediately obvious to me why this would be better than a good implementation on a single server that successfully manages TLS and unsecure connections in a way that prevents downgrade attacks. Some more context would be useful so that this sentence is interpreted as intended. If a recommendation is being made, perhaps "would be better served by" should be replaced by something with "MAY" in it.
ADRESSES, POSSIBLY.

TYPOS AND CLARITY

2.3 Peer: Change "the peer is the TACACS+ Server is the Client" to "the peer of the TACACS+ Server is the Client".
ADDRESSED

3 2. Peer Authentication: "The use of shared keys to add and remove the MD5 obfuscation..." in unclear.
ADDRESSED

3.2 TLS Connection: Missing full stop at end of third paragraph.
ADDRESSED

3.2.2.1 TLS Certificate Path Verification:

• Change "Path" to "path" in first sentence.
  ADDRESSED
• Change "entire chain of the remote's certificate is stored on the local Peer" to "entire chain of the remote Peer's certificate is stored on the local Peer."
  ADDRESSED
• Arguably the second paragraph is an operations consideration and should be removed or replaced by a reference to "6.4 Unreachable Certificate Authority (CA)". Alternatively, a cross reference could be added to the existing text.
  TBD

6.1 TLS Use: "Redundant lists" and "Server lists" are undefined and used inconsistently. Possible rephrasing is: "When introducing TLS to TACACS+ within a network, there is likely to be a period where there is a mixture of TACACS+ servers and/or port combination that support TLS and those that do not. During this period, a mixture of the two types of servers in a single list of TACACS+ servers configured on a client SHOULD be minimised. After migration, the production deployment SHOULD NOT mix the two types of servers within a list configured on a client."
ADDRESSES, POSSIBLY

6.3. Downgrade attacks in TLS: missing "... in" after "options".
REMOVED, THIS SECTION REVERTED

8. Discussion on Separate port vs Negotiated TLS: Change "it allows easy blocking the unobfuscated" to "it allows easy blocking of the unobfuscated".
   ADDRESSED.

Tidy based on Mohamed's comment:

That RFC says: "However, there is no IETF consensus on when separate ports should be used for secure and insecure variants of the same service [RFC2595] [RFC2817] [RFC6335]. The overall preference is for use of a single port, as noted in Section 6 of this document and Section 7.2 of [RFC6335], but the appropriate approach depends on the specific characteristics of the service."

Section 3.1 (In general)

The likely mode is that certificates will be issued using a domain name, not an IP address.
So, what about RFC9525 to compare an available domain name with the certificate?

"Implementations SHOULD support the TLS Server Name Indication
extension ([RFC6066], Section 3).
"

I wonder whether the requirement should be tweak as follows: clients SHOULD include the server domain name in the SNI extension?

Comment from Alan:

"Therefore, TLS Hello MUST be initiated ..."

What's a "Hello" ? Perhaps just say instead that the connection begins with TLS, and leave it at that.

"This document favors the predictable use of TLS security for a deployment"

I'm not sure what that means. Maybe "prefers" or "mandates" the use of TLS? I find the phrasing to be confusing.

Joe Clarke requests (suggests?) an Operator's Considerations section be added to describe interoperability with Legacy TACACS+

• configuring a fallback legacy T+ server along side a tacacss server (or servers),
• thoughts on migrating from a shared key to certificate-based validation,
• etc.

Document already addresses some of this in different parts of the document where it talks about PSKs, mixing legacy and tacacss on a single server, and the need to provide some port flexibility, but Joe thinks a centralized section might help bring focus to those migrating to this.

Section 3.2.2.1

"a.k.a. bundles or chains of trust"

What about RFC7924; should this be recommended to avoid transmitting the server's certificate and certificate chain if the client has cached that information from a previous TLS handshake?

Importing from haussli/draft-dahm-opsawg-tacacs-security#49

[dcmgashcisco] commented [on Apr 14]

the resumption ticket_lifetime SHOULD be configurable, including a zero seconds lifetime.

I'm not sure what a "zero-seconds lifetime" would mean. It may be better to just omit the ticket in that case.

Joe Clarke asked that failure to set the TAC_PLUS_UNENCRYPTED_FLAG be treated as an error rather than ignored. As a result, the server MUST return an failure, NOT an error, because an error could be interpreted as a server failure causing the client to try other servers OR to apply local AAA methods and thus could have unintended results. This is based on the description of FAIL vs. ERROR in S4.4 of RFC 8907. RFC8907 S10.5.2. Connections and Obfuscation seems to handle this case with a FAIL status.

A TAC_PLUS_ACCT_STATUS_FAIL status is needed.

Comment from Alan:
It would be good to give some explanation as to why MD5 is being removed.

Triage:
In the document we say merely: “The MD5 Obfuscation specified in the original protocol definition is not fit for purpose,”
We’ll provide some reference as to why.

The recent changes to the organisation & text have perverted this warning about mixing TLS and non-TLS connections, IMO.

This text
" In order to prevent downgrade attacks, Servers SHOULD keep separate and disjoint lists of clients supporting TLS and Unsecure Connections."

Is this conflating the warning about mixing TLS & non-TLS on clients and servers with the warning about the pitfalls of the servers or clients maintaining a learned list of clients/servers that support (or do not support) TLS[1]. Instead, it should just say that oil and water do not mix.

Perhaps it should be rewritten roughly as
" In order to prevent downgrade attacks, Servers SHOULD NOT offer both TLS and non-TLS connections on the same server [intentionally not capitalised; meaning "host"] and Clients should not configure both TLS and non-TLS Servers."

Later, the "lists" term is used in this text
"When Migrating from legacy service to TLS, any mixture of Unsecure Connected Servers and TLS-Protected Servers in the same redundant lists on clients SHOULD be minimised." and
" After migration, the production deployment SHOULD NOT mix Legacy and TLS-Protected Servers within Server lists configured on clients."

So, perhaps the previous use of the term "lists" is not referring to the warning about "learned lists". Either way, "lists" seems to be too vague. I do not have a good suggestion to make it clearer ATM.

[1] removed text was: "Servers and Clients could maintain a cache of Peers that have engaged in TACACS+ TLS connections and demand TLS from that point forward. ...."

Comment from Alan:

It may be useful to move the TLS-PSK discussion from 5.1.3 to here.

Though the RADEXT WG recently went through a long process of defining TLS-PSK for RADIUS. It wasn't trivial. See https://datatracker.ietf.org/doc/draft-ietf-radext-tls-psk/

If the document is going to say "TLS-PSK is possible" but nothing more, then I would suggest just forbidding it. There are a lot of details to get right with TLS-PSK.