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

feat: allow the use of insecure cipher suites if configured by user #492

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rsdmike
Copy link
Member

@rsdmike rsdmike commented Mar 4, 2025

feat: allow the use of insecure cipher suits if configured by user to support older devices.

addresses device-management-toolkit/console#445

@rsdmike rsdmike changed the title feat: allow the use of insecure cipher suits if configured by user feat: allow the use of insecure cipher suites if configured by user Mar 4, 2025
@@ -127,6 +127,13 @@ func NewWsman(cp Parameters) *Target {
config = res.tlsConfig
} else {
config = &tls.Config{InsecureSkipVerify: cp.SelfSignedAllowed}
if cp.AllowInsecureCipherSuites {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please update the TLS version to VersionTLS12 and use a more specific list of insecure ciphers instead of tls.InsecureCipherSuites()?

I've tested these changes on AMT v12.0.92.2145 and v11.8.86, and they work as expected. Here's the updated code for reference:

	if cp.AllowInsecureCipherSuites {
		config.MinVersion = tls.VersionTLS12

		for _, suite := range tls.CipherSuites() {
			config.CipherSuites = append(config.CipherSuites, suite.ID)
		}

		// add specific insecure ciphers
		insecureCiphers := []uint16{
			tls.TLS_RSA_WITH_AES_128_GCM_SHA256,
			tls.TLS_RSA_WITH_AES_128_CBC_SHA,
			tls.TLS_RSA_WITH_AES_256_CBC_SHA,
		}
		config.CipherSuites = append(config.CipherSuites, insecureCiphers...)
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

it didn't work with insecure ciphers? Those three are listed in the list: https://cs.opensource.google/go/go/+/refs/tags/go1.24.1:src/crypto/tls/cipher_suites.go;l=80

Copy link
Contributor

Choose a reason for hiding this comment

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

It does work, but I noticed that legacy AMT versions only support these specific ciphers. The trade-off is between limiting insecure ciphers for compatibility versus your approach of adding all insecure ciphers, which reduces maintenance if a cipher is marked as insecure in the future.

I'm okay with adding all the insecure ciphers. Let me know your thoughts!

@rsdmike rsdmike force-pushed the insecure branch 2 times, most recently from 3924259 to 73a56c6 Compare March 10, 2025 16:16
if configured by user to support older devices.

addresses device-management-toolkit/console#445
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

Successfully merging this pull request may close these issues.

2 participants