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

Set default TLS cipher suites if not configured #4067

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

Conversation

0xaravindh
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #4027

Special notes for your reviewer:

@github-actions github-actions bot added kind/cleanup Refactoring code, fixing up documentation, etc size/XL labels Dec 13, 2024
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@0xaravindh 0xaravindh self-assigned this Dec 13, 2024
@0xaravindh 0xaravindh marked this pull request as ready for review December 13, 2024 07:14
@0xaravindh 0xaravindh added the area/security Issues pertaining to security label Dec 13, 2024
@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: ff5beefd-f07d-49de-aee1-f32e911a2ea4

Status: FAILURE

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: 3b710f09-428c-4dde-b06b-ec8250cef565

Status: FAILURE

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 4e6ff41c-2d32-410f-a352-869b0be7aeaa

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4067/head:pr_4067 && git checkout pr_4067
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.46.0-dev-10e81f0

@0xaravindh 0xaravindh requested review from gongmax and igooch December 13, 2024 09:18
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@agones-bot
Copy link
Collaborator

Build Failed 😭

Build Id: fbbaa746-2822-4c00-9a3c-b74c19d38d1a

Status: FAILURE

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@agones-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 71577bc8-584b-4a1f-9b8c-61ef835cab70

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4067/head:pr_4067 && git checkout pr_4067
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.46.0-dev-188d1f9

@peterzhongyi
Copy link
Collaborator

Can you provide the steps that you use to test the changes locally, and verify the cipher suites no longer include "less secure options that hardcoded into the library", and provide some screenshots as proof?

@0xaravindh
Copy link
Member Author

I followed the steps from #4027 to reproduce the problem
you can see the proof in the attached screenshot

@peterzhongyi
Copy link
Collaborator

Hi @0xaravindh, for some reason I couldn't reproduce the cipher suite and it's specifically missing the TLS_RSA_WITH_3DES_EDE_CBC_SHA(and I didn't patch your PR). I asked a question in #4027 for some more clarification as well.

@0xaravindh
Copy link
Member Author

0xaravindh commented Jan 9, 2025

Hi @peterzhongyi

While using Agones v1.42.0, I am able to reproduce the issue. We are using go version 1.21.0 in that version

go 1.21

Microsoft recently disabling support for TLSv1.0 and TLSv1.1. This change has been reflected in Go 1.22.0, where the default minimum TLS version is set to TLS 1.2.
golang/go#62459

This issue will not reproduce the current version of Go (1.23.0) that we are using in Agones

cipher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security Issues pertaining to security kind/cleanup Refactoring code, fixing up documentation, etc size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable to configure cipher suites for the allocator
4 participants