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

Add fqdnCacheMinTTL configuration parameter #6808

Merged
merged 18 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion build/yamls/antrea-aks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4233,7 +4233,12 @@ data:
# Defaults to "". It must be a host string or a host:port pair of the DNS server (e.g. 10.96.0.10,
# 10.96.0.10:53, [fd00:10:96::a]:53).
dnsServerOverride: ""


# The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
# Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
# value of the application's DNS cache.
minTTL: ""

# Comma-separated list of Cipher Suites. If omitted, the default Go Cipher Suites will be used.
# https://golang.org/pkg/crypto/tls/#pkg-constants
# Note that TLS1.3 Cipher Suites cannot be added to the list. But the apiserver will always
Expand Down
7 changes: 6 additions & 1 deletion build/yamls/antrea-eks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4233,7 +4233,12 @@ data:
# Defaults to "". It must be a host string or a host:port pair of the DNS server (e.g. 10.96.0.10,
# 10.96.0.10:53, [fd00:10:96::a]:53).
dnsServerOverride: ""


# The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
# Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
# value of the application's DNS cache.
minTTL: ""

# Comma-separated list of Cipher Suites. If omitted, the default Go Cipher Suites will be used.
# https://golang.org/pkg/crypto/tls/#pkg-constants
# Note that TLS1.3 Cipher Suites cannot be added to the list. But the apiserver will always
Expand Down
7 changes: 6 additions & 1 deletion build/yamls/antrea-gke.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4233,7 +4233,12 @@ data:
# Defaults to "". It must be a host string or a host:port pair of the DNS server (e.g. 10.96.0.10,
# 10.96.0.10:53, [fd00:10:96::a]:53).
dnsServerOverride: ""


# The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
# Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
# value of the application's DNS cache.
minTTL: ""

# Comma-separated list of Cipher Suites. If omitted, the default Go Cipher Suites will be used.
# https://golang.org/pkg/crypto/tls/#pkg-constants
# Note that TLS1.3 Cipher Suites cannot be added to the list. But the apiserver will always
Expand Down
7 changes: 6 additions & 1 deletion build/yamls/antrea-ipsec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4246,7 +4246,12 @@ data:
# Defaults to "". It must be a host string or a host:port pair of the DNS server (e.g. 10.96.0.10,
# 10.96.0.10:53, [fd00:10:96::a]:53).
dnsServerOverride: ""


# The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
# Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
# value of the application's DNS cache.
minTTL: ""

# Comma-separated list of Cipher Suites. If omitted, the default Go Cipher Suites will be used.
# https://golang.org/pkg/crypto/tls/#pkg-constants
# Note that TLS1.3 Cipher Suites cannot be added to the list. But the apiserver will always
Expand Down
7 changes: 6 additions & 1 deletion build/yamls/antrea.yml
antoninbas marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -4233,7 +4233,12 @@ data:
# Defaults to "". It must be a host string or a host:port pair of the DNS server (e.g. 10.96.0.10,
# 10.96.0.10:53, [fd00:10:96::a]:53).
dnsServerOverride: ""


# The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
# Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
# value of the application's DNS cache.
minTTL: ""

# Comma-separated list of Cipher Suites. If omitted, the default Go Cipher Suites will be used.
# https://golang.org/pkg/crypto/tls/#pkg-constants
# Note that TLS1.3 Cipher Suites cannot be added to the list. But the apiserver will always
Expand Down
1 change: 1 addition & 0 deletions cmd/antrea-agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@ func run(o *Options) error {
nodeConfig,
podNetworkWait,
l7Reconciler,
o.minTTL,
)
if err != nil {
return fmt.Errorf("error creating new NetworkPolicy controller: %v", err)
Expand Down
5 changes: 5 additions & 0 deletions cmd/antrea-agent/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ type Options struct {
nplEndPort int
dnsServerOverride string
nodeType config.NodeType
minTTL uint32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this field doesn't seem necessary?


// enableEgress represents whether Egress should run or not, calculated from its feature gate configuration and
// whether the traffic mode supports it.
Expand Down Expand Up @@ -604,6 +605,10 @@ func (o *Options) validateK8sNodeOptions() error {
}
o.dnsServerOverride = hostPort
}
// If minTTL is greater than 1, it indicates that the value has been set externally by a cluster admin, and should be respected.
if o.config.MinTTL > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 1 and not 0?

o.minTTL = o.config.MinTTL
}

if err := o.validateSecondaryNetworkConfig(); err != nil {
return fmt.Errorf("failed to validate secondary network config: %v", err)
Expand Down
15 changes: 12 additions & 3 deletions pkg/agent/controller/networkpolicy/fqdn.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ type fqdnController struct {
ofClient openflow.Client
// dnsServerAddr stores the coreDNS server address, or the user provided DNS server address.
dnsServerAddr string
minTTL uint32

// dirtyRuleHandler is a callback that is run upon finding a rule out-of-sync.
dirtyRuleHandler func(string)
Expand Down Expand Up @@ -160,7 +161,7 @@ type fqdnController struct {
clock clock.Clock
}

func newFQDNController(client openflow.Client, allocator *idAllocator, dnsServerOverride string, dirtyRuleHandler func(string), v4Enabled, v6Enabled bool, gwPort uint32, clock clock.WithTicker) (*fqdnController, error) {
func newFQDNController(client openflow.Client, allocator *idAllocator, dnsServerOverride string, dirtyRuleHandler func(string), v4Enabled, v6Enabled bool, gwPort uint32, clock clock.WithTicker, minTTL uint32) (*fqdnController, error) {
controller := &fqdnController{
ofClient: client,
dirtyRuleHandler: dirtyRuleHandler,
Expand All @@ -182,6 +183,7 @@ func newFQDNController(client openflow.Client, allocator *idAllocator, dnsServer
ipv6Enabled: v6Enabled,
gwPort: gwPort,
clock: clock,
minTTL: minTTL,
}
if controller.ofClient != nil {
if err := controller.ofClient.NewDNSPacketInConjunction(dnsInterceptRuleID); err != nil {
Expand Down Expand Up @@ -636,22 +638,29 @@ func (f *fqdnController) parseDNSResponse(msg *dns.Msg) (string, map[string]ipWi
}
fqdn := strings.ToLower(msg.Question[0].Name)
responseIPs := map[string]ipWithExpiration{}
getMaxTTL := func(ttl1, ttl2 uint32) uint32 {
if ttl1 > ttl2 {
return ttl1
} else {
return ttl2
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a max built in in Golang: https://pkg.go.dev/builtin#max
It was introduced in Go 1.21

currentTime := f.clock.Now()
for _, ans := range msg.Answer {
switch r := ans.(type) {
case *dns.A:
if f.ipv4Enabled {
responseIPs[r.A.String()] = ipWithExpiration{
ip: r.A,
expirationTime: currentTime.Add(time.Duration(r.Header().Ttl) * time.Second),
expirationTime: currentTime.Add(time.Duration(getMaxTTL(f.minTTL, r.Header().Ttl)) * time.Second),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like technically, minTTL should only apply to DNS responses which were not initiated by Antrea, but intercepted by Antrea (responses to DNS queries generated by the application). However, this code applies to responses to DNS queries sent by Antrea (when an override DNS server is configured). Maybe we need to introduce a flag to distinguish between the 2 cases?

cc @tnqn @Dyanngg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually spent the length of last syncup meeting to discuss this, because I had the same suggestion but @tnqn’s opinion was that we don’t have to differentiate between the two cases. For security purpose we advise users to use FQDN rules only in allowlists for Antrea-native policies, and he thinks it’s okay that the clients’ TTL for a FQDN goes “out of sync” with the antrea agent since that’s not the gaurentee we want: we only want to enforce that client cannot access unintended addresses. So having an address for a domain which has longer TTL in antrea cache compared to the client is ok. I’ll let Quan chime in to see if I’m summarizing this correctly, but the end result of the discussion was we told Hemant to not worry about differentiating Antrea and client initiated dns queries

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For security purpose we advise users to use FQDN rules only in allowlists for Antrea-native policies

Got it, makes sense to me

@hkiiita please ignore this comment

}

}
case *dns.AAAA:
if f.ipv6Enabled {
responseIPs[r.AAAA.String()] = ipWithExpiration{
ip: r.AAAA,
expirationTime: currentTime.Add(time.Duration(r.Header().Ttl) * time.Second),
expirationTime: currentTime.Add(time.Duration(getMaxTTL(f.minTTL, r.Header().Ttl)) * time.Second),
}
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/agent/controller/networkpolicy/fqdn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func newMockFQDNController(t *testing.T, controller *gomock.Controller, dnsServe
false,
config.DefaultHostGatewayOFPort,
clockToInject,
0,
)
require.NoError(t, err)
return f, mockOFClient
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func NewNetworkPolicyController(antreaClientGetter client.AntreaClientProvider,
gwPort, tunPort uint32,
nodeConfig *config.NodeConfig,
podNetworkWait *utilwait.Group,
l7Reconciler *l7engine.Reconciler) (*Controller, error) {
l7Reconciler *l7engine.Reconciler, minTTL uint32) (*Controller, error) {
idAllocator := newIDAllocator(asyncRuleDeleteInterval, dnsInterceptRuleID)
c := &Controller{
antreaClientProvider: antreaClientGetter,
Expand Down Expand Up @@ -227,7 +227,7 @@ func NewNetworkPolicyController(antreaClientGetter client.AntreaClientProvider,

var err error
if antreaPolicyEnabled {
if c.fqdnController, err = newFQDNController(ofClient, idAllocator, dnsServerOverride, c.enqueueRule, v4Enabled, v6Enabled, gwPort, clock.RealClock{}); err != nil {
if c.fqdnController, err = newFQDNController(ofClient, idAllocator, dnsServerOverride, c.enqueueRule, v4Enabled, v6Enabled, gwPort, clock.RealClock{}, minTTL); err != nil {
return nil, err
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ func newTestController() (*Controller, *fake.Clientset, *mockReconciler) {
config.DefaultTunOFPort,
&config.NodeConfig{},
wait.NewGroup(),
l7reconciler)
l7reconciler,
0)
reconciler := newMockReconciler()
controller.podReconciler = reconciler
controller.auditLogger = nil
Expand Down
4 changes: 4 additions & 0 deletions pkg/config/agent/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ type AgentConfig struct {
// Defaults to "". It must be a host string or a host:port pair of the DNS server (e.g. 10.96.0.10,
// 10.96.0.10:53, [fd00:10:96::a]:53).
DNSServerOverride string `yaml:"dnsServerOverride,omitempty"`
// The minTTL setting helps address the problem of applications caching DNS response IPs indefinitely.
// The Cluster administrators should configure this value, ideally setting it to be equal to or greater than the maximum TTL
// value of the application's DNS cache.
MinTTL uint32 `yaml:"minTTL,omitempty"`
antoninbas marked this conversation as resolved.
Show resolved Hide resolved
// Cipher suites to use.
TLSCipherSuites string `yaml:"tlsCipherSuites,omitempty"`
// TLS min version.
Expand Down
Loading