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

Store Redis username & password in a dedicated environment variables #2083

Closed
AJIOB opened this issue Feb 9, 2024 · 7 comments · Fixed by #2116
Closed

Store Redis username & password in a dedicated environment variables #2083

AJIOB opened this issue Feb 9, 2024 · 7 comments · Fixed by #2116

Comments

@AJIOB
Copy link
Contributor

AJIOB commented Feb 9, 2024

As I can see, this split was done in a manner that opendal processes URL prefixes (out of the current issue scope).

But this concept is not useful, IMO.

I prefer to pass a single SCCACHE_WEBDAV_ENDPOINT and extract SCCACHE_WEBDAV_KEY_PREFIX from the endpoint. If opendal needs, SCCACHE_WEBDAV_ENDPOINT value may be removed from SCCACHE_WEBDAV_KEY_PREFIX too (for better understanding/support via opendal provider).

If both SCCACHE_WEBDAV_ENDPOINT path is non-root and SCCACHE_WEBDAV_ENDPOINT is not empty, we should:

  1. Pass them without any changes to the opendal
  2. Check via an assert! and finish the program as an invalid case

I prefer to use the second variant, but maybe some implementations exist that requires both of the values (I don't know anything about them).

@AJIOB
Copy link
Contributor Author

AJIOB commented Feb 9, 2024

P.S. Username & password may be passed as a part of the SCCACHE_WEBDAV_ENDPOINT too (optionally), because it is still a valid URL.
P.P.S. The Redis backend connection parameters are passed in the same manner that I suggest to WebDAV.

@Xuanwo
Copy link
Collaborator

Xuanwo commented Feb 9, 2024

The prefix is based on input endpoint.

Some webdav services could expose webdav at /v1/webdav, so it might be confusing to extract them as path.

Username & password may be passed as a part of the SCCACHE_WEBDAV_ENDPOINT too (optionally), because it is still a valid URL.

Although the URL is technically valid, incorporating the password directly within the endpoint is risky, as the endpoint is not excluded from Debug output. This oversight could potentially expose the password in logs or traces, leading to a security vulnerability.

@AJIOB
Copy link
Contributor Author

AJIOB commented Feb 9, 2024

Some webdav services could expose webdav at /v1/webdav, so it might be confusing to extract them as path.

As I can see in Artifactory docs - just used as an example - I should use artifactory/repo-path as a part of the SCCACHE_WEBDAV_ENDPOINT or as a SCCACHE_WEBDAV_KEY_PREFIX value now?

@AJIOB
Copy link
Contributor Author

AJIOB commented Feb 9, 2024

Although the URL is technically valid, incorporating the password directly within the endpoint is risky, as the endpoint is not excluded from Debug output. This oversight could potentially expose the password in logs or traces, leading to a security vulnerability.

Make sense. Let's provide additional variables for the username & password for the Redis. Will it be OK?

@Xuanwo
Copy link
Collaborator

Xuanwo commented Feb 9, 2024

As I can see in Artifactory docs - just used as an example - I should use artifactory/repo-path as a part of the SCCACHE_WEBDAV_ENDPOINT or as a SCCACHE_WEBDAV_KEY_PREFIX value now?

From my perspective, I believe the endpoint should be http://host:port/artifactory/repo-path, a valid WebDAV service that points to the top-level directory accessible by users. With this endpoint as a basis, we can determine the key prefix and more.

Let's provide additional variables for the username & password for the Redis. Will it be OK?

LGTM. We should encourage users to use the username and password provided by the config instead of directly in the URL.

@AJIOB
Copy link
Contributor Author

AJIOB commented Feb 9, 2024

LGTM. We should encourage users to use the username and password provided by the config instead of directly in the URL.

You prefer to drop the current format support for the new one or just support both or them?

@Xuanwo
Copy link
Collaborator

Xuanwo commented Feb 9, 2024

You prefer to drop the current format support for the new one or just support both or them?

Dropping current format might lead to a breaking change which is not good. I prefer to support both by including username and password in the config. We can also add a note in our documentation, advising new users that using password is recommended.

@AJIOB AJIOB changed the title Extract SCCACHE_WEBDAV_KEY_PREFIX value from SCCACHE_WEBDAV_ENDPOINT Store Redis username & password in a dedicated environment variables Feb 15, 2024
AJIOB added a commit to AJIOB/sccache that referenced this issue Feb 27, 2024
* Store Redis credentials via dedicated fields: for security reasons, as discussed in mozilla#2083
* Deprecate `url` usage for Redis config
Xuanwo pushed a commit that referenced this issue Feb 28, 2024
* Store Redis credentials via dedicated fields: for security reasons, as discussed in #2083
* Deprecate `url` usage for Redis config
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 a pull request may close this issue.

2 participants