Presence of AppChannelAllowInsecureTLS feature flag

[fazledyn-or]

In what area(s)?

/area runtime

What version of Dapr?

1.12.0

Expected Behavior

While triaging your project, our bug fixing tool generated the following message-

In channels.go, there is a method appHTTPClient that uses insecure protocol version in TLS configuration.

Details

The bug fixing tool generated the following diff -

@@ -374,6 +374,8 @@
 			}
 			// TODO: Remove when the feature is finalized
 			if globalConfig.IsFeatureEnabled(config.AppChannelAllowInsecureTLS) {
+				// OpenRefactory Warning:
+				// Usage of insecure protocol version in TLS configuration.
 				tlsConfig.MinVersion = 0
 			}
 		}

From the comments at channels.go and configuration.go, I can see that the feature flag AppChannelAllowInsecureTLS is to be removed in Dapr 1.13. However, in the same configuration.go file, we can see that ActorStateTTL feature flag is present, where it says -

	// Enables support for setting TTL on Actor state keys. Remove this flag in
	// Dapr 1.12.
	ActorStateTTL Feature = "ActorStateTTL"

Since Dapr 1.12 was release last month on Oct 12, 2023 and the flag hasn't been removed, looks like it's a mistake.

In that case -

1. Shouldn't this feature flag be removed in the next release (v1.13)?
2. Shouldn't the feature flag AppChannelAllowInsecureTLS be removed too?

If the AppChannelAllowInsecureTLS flag is to stay, then it leads a security flaw since the crypto/tls documentation states that -

	// MinVersion contains the minimum TLS version that is acceptable.
	//
	// By default, TLS 1.2 is currently used as the minimum when acting as a
	// client, and TLS 1.0 when acting as a server. TLS 1.0 is the minimum
	// supported by this package, both as a client and as a server.
	//

Source: https://pkg.go.dev/crypto/tls#Config.MinVersion

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

Actual Behavior

Steps to Reproduce the Problem

Release Note

RELEASE NOTE:

[ItalyPaleAle]

I agree, this should have been removed

@DeepanshuA you worked on this right?

[DeepanshuA]

@fazledyn-or Thanks for raising this up.

@ItalyPaleAle I will pick this up

[DeepanshuA]

/assign

[msfussell]

@mukundansundar - Does this affect docs?

[mukundansundar]

I do not think so. This a breaking change which will be in the breaking changes section in release notes.
This flag is not a preview features flag which actually is present in a section in docs.
Unless we mention TLS min versions in docs, we will not have any changes in docs.
@DeepanshuA wdyt?

[elena-kolevska]

Any updates on this regarding the docs?

[DeepanshuA]

Yeah, it should be in breaking changes section in docs, where-in we just need to specify that we can't use anymore TLS below 1.2 for App Channel - even the AppChannelAllowInsecureTLS is not supported anymore.

Can you please point me to the 1.13 Release Notes hackmd link, I can make a change in breaking change section.

[mukundansundar]

Yeah, it should be in breaking changes section in docs, where-in we just need to specify that we can't use anymore TLS below 1.2 for App Channel - even the AppChannelAllowInsecureTLS is not supported anymore.

Can you please point me to the 1.13 Release Notes hackmd link, I can make a change in breaking change section.

If this is only in the breaking changes section, that is covered by the label breaking-change when the release notes is generated.