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

Support for minimum TLS version for opa server #3517

Merged
merged 4 commits into from
Jun 30, 2021

Conversation

kale-amruta
Copy link
Contributor

Opa server now supports minimum TLS version. Since TLS 1.0 and 1.1 are deprecated, default minimum TLS version for opa server is TLS 1.2 . If someone wants to restrict opa to start with a specific minimum tls version, they can specify it using cmd line parameter --min-tls-version. TLS versions supported are 1.0, 1.1, 1.2, 1.3.

fixes #3226
Signed-off-by: Amruta Kale amruta.kale@styra.com

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

😃 That's great, I like this. 🚀

Some questions and nitpicks inline 🙃

cmd/run.go Outdated Show resolved Hide resolved
server/server.go Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
test/e2e/tls/tls_test.go Show resolved Hide resolved
runtime/runtime.go Outdated Show resolved Hide resolved
@kale-amruta kale-amruta force-pushed the amruta-3226 branch 2 times, most recently from eec8ea5 to 2997f32 Compare June 10, 2021 08:48
@kale-amruta kale-amruta requested a review from srenatus June 10, 2021 08:50
@kale-amruta
Copy link
Contributor Author

@srenatus @anderseknert does this look good?

@srenatus
Copy link
Contributor

@kale-amruta having another look just now 😃

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with me, this looks good!

Besides some tiny newline nitpicks, I'm wondering about the use of flags in the e2e tests, could you have a look at my inline comments, please? 😃

runtime/runtime.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
test/e2e/tls/tls_test.go Outdated Show resolved Hide resolved
test/e2e/tls/tls_test.go Outdated Show resolved Hide resolved
// print error to stderr, exit 1
func fatal(err interface{}) {
fmt.Fprintf(os.Stderr, "%s\n", err)
os.Exit(1)
}

func TestMain(m *testing.M) {
minTLSVersion := flag.String("--min-TLS-Version", "1.2", "minimum TLS Version")
TLSVersion := minTLSVersions[*minTLSVersion]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrmm I don't quite understand why we're going through flag here. It seems quite indirect, and if this is why we need to mess with os.Args below, I'd rather keep looking for a different solution. (If this style is consistent with the other e2e tests, which I haven't check yet, please ignore me 😅)

I suppose we could set testServerParams.MinTLSVersion directly. If we need a different value for the other test below, could we change that field...?

Copy link
Contributor Author

@kale-amruta kale-amruta Jun 29, 2021

Choose a reason for hiding this comment

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

@srenatus all the tests in tls_test.go run through same opa server instance.
We have two set of testcases -

  1. default tls version set for server
  2. TLS1.3 set for server as implemented here
    Hence to pass minTLSVersion to testServerParams through TestMain at runtime, I had to use os.Args, else I would have had to create two different servers for these testcases

Note: This flag is different from the one defined on server, its added to run the test with the command line parameter

Amruta Kale added 3 commits June 29, 2021 16:41
Opa server now supports min TLS version, TLS versions supported are 1.0, 1.1, 1.2, 1.3.
Since TLS 1.0 and 1.1 are deprecated, default min TLS version for opa is TLS 1.2 but
if someone wants to restrict opa to use a specific minimum TLS version, they can specify it using cmd parameter --min-tls-version

fixes open-policy-agent#3226
Signed-off-by: Amruta Kale <amruta.kale@styra.com>
Signed-off-by: Amruta Kale <amruta.kale@styra.com>
Signed-off-by: Amruta Kale <amruta.kale@styra.com>
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Will merge when green. Thanks for implementing this, it's great to have such a feature with a good default 👍

@kale-amruta
Copy link
Contributor Author

awesome. Thanks @srenatus

@srenatus srenatus merged commit 55db837 into open-policy-agent:main Jun 30, 2021
@kale-amruta kale-amruta deleted the amruta-3226 branch June 30, 2021 08:02
juliafriedman8 pushed a commit to juliafriedman8/opa that referenced this pull request Jul 13, 2021
…y-agent#3517)

* Support for minimum TLS version

OPA server now supports min TLS version, TLS versions supported are 1.0, 1.1, 1.2, 1.3.

Since TLS 1.0 and 1.1 are deprecated, default min TLS version for OPA is TLS 1.2 but
if someone wants to restrict OPA to use a specific minimum TLS version, they can
specify it using cmd parameter `--min-tls-version`.

Fixes open-policy-agent#3226.

Signed-off-by: Amruta Kale <amruta.kale@styra.com>
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.

Support TLS minVersion and maxVersion in opa server so that it can disable TLS 1.0 and 1.1
3 participants