Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

CA-371790: Restrict the permissions on pool tokens #4857

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

snwoods
Copy link
Contributor

@snwoods snwoods commented Nov 24, 2022

Depends on the changes in xapi-project/stdext#69

@snwoods snwoods force-pushed the private/stevenwo/CA-371790 branch from 006b62a to e939b3c Compare November 24, 2022 17:33
@lindig
Copy link
Contributor

lindig commented Nov 25, 2022

File "ocaml/xapi/helpers.ml", line 1954, characters 4-30:
1954 |     SecretString.write_to_file ~perms:0o600 !Xapi_globs.pool_secret_path ps ;
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This function has type string -> SecretString.t -> unit
       It is applied to too many arguments; maybe you forgot a `;'.
make: *** [Makefile:16: build] Error 1
Error: Process completed with exit code 2.

@robhoes
Copy link
Member

robhoes commented Nov 25, 2022

We first need to get the stdext change reviewed/committed and added to xs-opam.

@robhoes
Copy link
Member

robhoes commented Nov 25, 2022

You also need a make format.

@snwoods snwoods force-pushed the private/stevenwo/CA-371790 branch from e939b3c to 5164cb9 Compare November 25, 2022 13:34
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

LGTM, please wait to merge until xs-opam includes the necessary code to make the code compile

Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

I don't like that users of the function can dictate the permissions, please undo those changes and instead change the implementation of the function to hardcode it:

let write_to_file = Xapi_stdext_unix.Unixext.write_string_to_file ~perms:0o600

@lindig
Copy link
Contributor

lindig commented Nov 30, 2022

I don't like that users of the function can dictate the permissions, please undo those changes and instead change the implementation of the function to hardcode it:

let write_to_file = Xapi_stdext_unix.Unixext.write_string_to_file ~perms:0o600

Is this based on the argument that we already know that we want to store a secret and hence need most restrictive permissions? Which perms don't you like? I think passing permission to Xapi_stdext_unix.Unixext.write_string_to_file is good, but to SecretString.write_to_file maybe not.

@psafont
Copy link
Member

psafont commented Nov 30, 2022

Is this based on the argument that we already know that we want to store a secret and hence need most restrictive permissions? Which perms don't you like?

Yes, since it's a secrets the permissions need to be more stringent. Currently it's on SecretString's user to use the safe permissions, this is risky since it's done a optional parameter which might be elided. Even if it wasn't I don't think it should be a decision at all as it should always use the safe choice, hence why it only makes sense to encode it in SecretString.write_to_file and not expose it to its users

Signed-off-by: Steven Woods <steven.woods@citrix.com>
@snwoods snwoods force-pushed the private/stevenwo/CA-371790 branch from 5164cb9 to 53215ea Compare December 1, 2022 11:05
@psafont psafont merged commit 34eae31 into xapi-project:master Dec 1, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants