Issue 479: Addressing Security vulnerabilities with authentication #488

Prabhaker24 · 2020-12-18T02:30:20Z

Change log description

This pr Addresses the Security Vulnerability when Authentication is enabled for pravega.

Purpose of the change

Fixes #479

What the code does

This pr allows the user to specify tokensecrets for both segmentstore and controller and it mounts those secret in a file mounted at /etc/controller-auth-volume for controller and at /etc/ss-auth-volume for segmentstore. Following Params are specified in the secret:-

controller.security.auth.delegationToken.signingKey.basis
autoScale.security.auth.token.signingKey.basis
pravega.client.auth.token
pravega.client.auth.method
controller.connect.auth.credentials.dynamic

controller secret is created from a file containing only controller.security.auth.delegationToken.signingKey.basis as delegationToken.signingKey.basis
eg:-

delegationToken.signingKey.basis: {secret}

Segmentstore secret contains autoScale.security.auth.token.signingKey.basis as delegationToken.signingKey.basis as well as pravega.client.auth.method ,pravega.client.auth.token and controller.connect.auth.credentials.dynamic are specified as controller.connect.auth.params and are separated by semicolon and in the same order.

eg:-

delegationToken.signingKey.basis: {secret}
controller.connect.auth.params: {method};{token};{dynamic}

How to verify it

After pravegacluster is deployed exec into the ss and controller pod and check for the presence of the respective files at the above-specified location for both ss and controller containing the values given by there secret respectively.

Signed-off-by: prabhaker24 <[email protected]>

…into Issue-479-security-vulnerabilities-with-Authentication

codecov-io · 2020-12-18T02:32:46Z

Codecov Report

Merging #488 (4333ee9) into master (e6dc8e3) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   71.62%   71.77%   +0.14%     
==========================================
  Files          15       15              
  Lines        3411     3429      +18     
==========================================
+ Hits         2443     2461      +18     
  Misses        865      865              
  Partials      103      103

Impacted Files	Coverage Δ
pkg/apis/pravega/v1beta1/pravegacluster_types.go	`25.36% <ø> (ø)`
pkg/controller/pravega/pravega_controller.go	`100.00% <100.00%> (ø)`
pkg/controller/pravega/pravega_segmentstore.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6dc8e3...4333ee9. Read the comment docs.

sarlaccpit · 2021-01-05T16:56:55Z

pkg/apis/pravega/v1beta1/pravegacluster_types.go

@@ -290,6 +290,12 @@ type AuthenticationParameters struct {
 	// name of Secret containing Password based Authentication Parameters like username, password and acl
 	// optional - used only by PasswordAuthHandler for authentication
 	PasswordAuthSecret string `json:"passwordAuthSecret,omitempty"`
+


For integration purposes, there needs to be a clear description of how to create the two secrets.
I can see through the code the format of the secret is straight forward, with one having one entry which is just a random string; and the other having two entries, one being the same random string, and one being a base64 encoded string of the admin/password, to be used with the password handler.

It won't be obvious to the common dweller though. :)

Having clear couple kubectl commands in the README that show exactly how to generate these 2 secrets is crucial so integrators know how to leverage this welcome enhancement.

Thanks @sarlaccpit, I have added documentation in the auth section of our documentation.

Thanks, the auth.md is very useful and detailed. It will be crucial information.

Thanks @sarlaccpit .

Signed-off-by: prabhaker24 <[email protected]>

…into Issue-479-security-vulnerabilities-with-Authentication

sarlaccpit · 2021-01-22T16:10:33Z

pkg/controller/pravega/pravega_controller_test.go

-						PasswordAuthSecret: "authentication-secret",
+						Enabled:               true,
+						PasswordAuthSecret:    "authentication-secret",
+						ControllerTokenSecret: "controllerauthsecret",


For consistency, maybe controllerauthsecret --> controllerauth-secret ?

I have changed it.

sarlaccpit · 2021-01-22T16:12:24Z

doc/auth.md

+
+Below is how we can create secret and expose them as file for Auth related properties:-
+
+1. Create a File containg `controller.security.auth.delegationToken.signingKey.basis`  as `delegationToken.signingKey.basis` which represent the tokensigning key used to connect to the controller:


Just a nit but I'm not getting what the "basis" suffix means?

Pravega uses this suffix so I tried keeping it the same. In any case this is just a key in a secret so if pravega changes the name for this parameter we can always have the name of the key different.

sarlaccpit · 2021-01-22T16:12:48Z

pkg/apis/pravega/v1beta1/pravegacluster_types.go

@@ -290,6 +290,12 @@ type AuthenticationParameters struct {
 	// name of Secret containing Password based Authentication Parameters like username, password and acl
 	// optional - used only by PasswordAuthHandler for authentication
 	PasswordAuthSecret string `json:"passwordAuthSecret,omitempty"`
+


Thanks, the auth.md is very useful and detailed. It will be crucial information.

sarlaccpit · 2021-01-22T16:13:13Z

doc/auth.md

+
+1. Create a File containg `controller.security.auth.delegationToken.signingKey.basis`  as `delegationToken.signingKey.basis` which represent the tokensigning key used to connect to the controller:
+
+Sample encrypted password file:


Line 89 was maybe a left over from a copy paste from somewhere?

I have removed it now.

Signed-off-by: prabhaker24 <[email protected]>

pkg/controller/pravega/pravega_controller_test.go

Co-authored-by: Christophe Balczunas <[email protected]>

sarlaccpit

Looks good. Just one small misspelled secret name in the tests.

Please insure that Pravega can be deployed without specifying these secrets. So that backward compatibility and smooth adoption of this feature can happen.

Prabhaker24 · 2021-01-27T16:46:07Z

Looks good. Just one small misspelled secret name in the tests.

Please insure that Pravega can be deployed without specifying these secrets. So that backward compatibility and smooth adoption of this feature can happen.

I have tested both ways of taking the auth parameters i.e as jvm options and as secret works from the operator side.

sarlaccpit · 2021-01-27T16:47:21Z

Looks good. Just one small misspelled secret name in the tests.
Please insure that Pravega can be deployed without specifying these secrets. So that backward compatibility and smooth adoption of this feature can happen.

I have tested both ways of taking the auth parameters i.e as jvm options and as secret works from the operator side.

Awesome. Just for completeness, is there a pravega github issue for the leveraging of these secrets?

Prabhaker24 · 2021-01-27T16:56:34Z

Looks good. Just one small misspelled secret name in the tests.
Please insure that Pravega can be deployed without specifying these secrets. So that backward compatibility and smooth adoption of this feature can happen.

I have tested both ways of taking the auth parameters i.e as jvm options and as secret works from the operator side.

Awesome. Just for completeness, is there a pravega github issue for the leveraging of these secrets?

link to the issue at pravega side pravega/pravega#5590.

prabhaker24 added 3 commits December 4, 2020 15:27

initial commit

7d5e702

Signed-off-by: prabhaker24 <[email protected]>

added support for adding token as secret

4755baa

Signed-off-by: prabhaker24 <[email protected]>

Merge branch 'master' of https://github.com/pravega/pravega-operator …

df6da84

…into Issue-479-security-vulnerabilities-with-Authentication

Prabhaker24 requested review from anishakj and Ranganaths8 December 18, 2020 02:30

sarlaccpit reviewed Jan 5, 2021

View reviewed changes

prabhaker24 added 4 commits January 13, 2021 10:23

added changes for exposing secret as files added

aed02aa

Signed-off-by: prabhaker24 <[email protected]>

doc and ut added

7725877

Signed-off-by: prabhaker24 <[email protected]>

fixed typo

81906e5

Signed-off-by: prabhaker24 <[email protected]>

Merge branch 'master' of https://github.com/pravega/pravega-operator …

fe86d8d

…into Issue-479-security-vulnerabilities-with-Authentication

sarlaccpit reviewed Jan 22, 2021

View reviewed changes

addressed comments

4333ee9

Signed-off-by: prabhaker24 <[email protected]>

sarlaccpit reviewed Jan 27, 2021

View reviewed changes

pkg/controller/pravega/pravega_controller_test.go Outdated Show resolved Hide resolved

Update pkg/controller/pravega/pravega_controller_test.go

f72f419

Co-authored-by: Christophe Balczunas <[email protected]>

sarlaccpit approved these changes Jan 27, 2021

View reviewed changes

Prabhaker24 mentioned this pull request Jan 27, 2021

Adding support for reading sensitive information(Auth paramaters) from a file instead of reading them from jvmOptions pravega/pravega#5590

Open

anishakj merged commit 9d15bf1 into master Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 479: Addressing Security vulnerabilities with authentication #488

Issue 479: Addressing Security vulnerabilities with authentication #488

Prabhaker24 commented Dec 18, 2020 •

edited

Loading

codecov-io commented Dec 18, 2020 •

edited

Loading

sarlaccpit Jan 5, 2021

Prabhaker24 Jan 16, 2021 •

edited

Loading

sarlaccpit Jan 22, 2021

Prabhaker24 Jan 25, 2021

sarlaccpit Jan 22, 2021

Prabhaker24 Jan 25, 2021

sarlaccpit Jan 22, 2021

Prabhaker24 Jan 25, 2021 •

edited

Loading

sarlaccpit Jan 22, 2021

sarlaccpit Jan 22, 2021

Prabhaker24 Jan 25, 2021

sarlaccpit left a comment

Prabhaker24 commented Jan 27, 2021

sarlaccpit commented Jan 27, 2021

Prabhaker24 commented Jan 27, 2021


		Below is how we can create secret and expose them as file for Auth related properties:-

		1. Create a File containg `controller.security.auth.delegationToken.signingKey.basis` as `delegationToken.signingKey.basis` which represent the tokensigning key used to connect to the controller:


		1. Create a File containg `controller.security.auth.delegationToken.signingKey.basis` as `delegationToken.signingKey.basis` which represent the tokensigning key used to connect to the controller:

		Sample encrypted password file:

Issue 479: Addressing Security vulnerabilities with authentication #488

Issue 479: Addressing Security vulnerabilities with authentication #488

Conversation

Prabhaker24 commented Dec 18, 2020 • edited Loading

Change log description

Purpose of the change

What the code does

How to verify it

codecov-io commented Dec 18, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Prabhaker24 Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prabhaker24 Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarlaccpit left a comment

Choose a reason for hiding this comment

Prabhaker24 commented Jan 27, 2021

sarlaccpit commented Jan 27, 2021

Prabhaker24 commented Jan 27, 2021

Prabhaker24 commented Dec 18, 2020 •

edited

Loading

codecov-io commented Dec 18, 2020 •

edited

Loading

Prabhaker24 Jan 16, 2021 •

edited

Loading

Prabhaker24 Jan 25, 2021 •

edited

Loading