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

3078 use otp secret type #3202

Merged
merged 14 commits into from
Apr 6, 2023
Merged

3078 use otp secret type #3202

merged 14 commits into from
Apr 6, 2023

Conversation

mssalvatore
Copy link
Collaborator

@mssalvatore mssalvatore commented Apr 6, 2023

What does this PR do?

Uses a SecretStr everywhere OTPs are used. This is a defensive coding measure to prevent OTPs from being accidentally leaked into the logs.

Note

The ISecretVariable was added to attempt to isolate other components from pydantic. While a noble attempt, the interface is too loose to be truly valuable (get_secret_value() returns Any). The OTP type alias itself serves the purpose of isolating the code from pydantic. The only pydantic "leakage" would be the name of the method, get_secret_value(). However, if we removed pydantic, as long as whatever replaced pydantic.SecretStr still provides get_secret_value() -> str, no other code needs to change.

We could provide ISecretStr, but if its interface is identical to SecretStr, I don't see what value that would offer us.

An additional note: We'd prefer OTP to be a concrete type so that strings can be converted to OTPs easily with OTP(my_otp_str).

Scope

We need to make the same changes for the Token type, but that is out of scope for this PR.

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?
  • Have you checked that you haven't introduced any duplicate code?

Testing Checklist

  • Added relevant unit tests?
  • Do all unit tests pass?
  • Do all end-to-end tests pass?
  • Any other testing performed?
  • If applicable, add screenshots or log transcripts of the feature working

The `ISecretVariable` was added to attempt to isolate other components
from pydantic. While a noble attempt, the interface is too loose to be
truly valuable (get_secret_value() returns Any). The OTP type alias
itself serves the purpose of isolating the code from pydantic. The only
pydantic "leakage" would be the name of the method,
`get_secret_value()`. However, if we removed pydantic, as long as
whatever replaced `pydantic.SecretStr` still provides
`get_secret_value() -> str`, no other code needs to change.

We could provide `ISecretStr`, but if its interface is identical to
`SecretStr`, I don't see what value that would offer us.

An additional note: We'd prefer OTP to be a concrete type so that
strings can be converted to OTPs easily with `OTP(my_otp_str)`.
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.03 ⚠️

Comparison is base (beee177) 73.05% compared to head (7539afd) 73.02%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3202      +/-   ##
===========================================
- Coverage    73.05%   73.02%   -0.03%     
===========================================
  Files          471      469       -2     
  Lines        13585    13566      -19     
===========================================
- Hits          9924     9907      -17     
+ Misses        3661     3659       -2     

see 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mssalvatore mssalvatore merged commit 01dcc4f into develop Apr 6, 2023
@mssalvatore mssalvatore deleted the 3078-use-otp-secret-type branch April 6, 2023 11:12
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.

3 participants