fix for floconsole and kms#288
Closed
vizsatiz wants to merge 2 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughKMS now exposes a jwt_algorithm; FloKmsService proxies it. GCP KMS detects PKCS#1 vs PSS and chooses padding. TokenService resolves the signing algorithm via KMS or config, expands decode allowlist (e.g., RS256 ± PS256), and raises on invalid signatures. A script demonstrates the end-to-end flow. ChangesJWT Algorithm Resolution and Signature Verification
Sequence DiagramsequenceDiagram
participant Client
participant TokenService
participant FloKmsService
participant GcpKMS
Client->>TokenService: create_token(claims)
TokenService->>FloKmsService: jwt_algorithm()
FloKmsService->>GcpKMS: jwt_algorithm()
GcpKMS-->>FloKMSService: RS256 or PS256
FloKMSService-->>TokenService: algorithm
TokenService->>GcpKMS: sign(message)
GcpKMS-->>TokenService: signature
TokenService-->>Client: token
Client->>TokenService: decode_token(jwt)
TokenService->>FloKmsService: jwt_algorithm() (for allowlist)
TokenService->>GcpKMS: get public key PEM
GcpKMS-->>TokenService: public key
TokenService->>GcpKMS: verify(message, signature)
GcpKMS-->>TokenService: True/False
TokenService-->>Client: decoded claims or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| check=True, | ||
| capture_output=True, | ||
| ) | ||
| priv_pem = open(priv.name, 'rb').read() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wavefront/server/modules/auth_module/auth_module/services/token_service.py (1)
30-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType annotation does not match runtime handling.
The parameter is typed as
FloKMSbut line 38 explicitly handles theNonecase (kms_service is None). This should beFloKMS | Noneto match the floconsole implementation and avoid type checker warnings.Suggested fix
- kms_service: FloKMS, + kms_service: FloKMS | None,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wavefront/server/modules/auth_module/auth_module/services/token_service.py` at line 30, Update the type annotation for the kms_service parameter to allow None: change its annotation from FloKMS to FloKMS | None where it is declared (the parameter named kms_service in token_service.py), so the signature matches the runtime check `if kms_service is None` (and mirrors floconsole). Ensure any internal references treat kms_service as optional or perform the existing None check before use.
🧹 Nitpick comments (1)
wavefront/server/packages/flo_cloud/flo_cloud/gcp/kms.py (1)
52-56: 💤 Low valueConsider caching the public key PEM during initialization.
The public key is fetched twice: once here to get the algorithm, and again in
get_public_key_pem()whenverify()is called. You could cachepublic_key.pemduring init to avoid the redundant API call.♻️ Optional optimization
public_key = self.kms_client.get_public_key( request=kms_v1.GetPublicKeyRequest(name=self.key_name) ) self._key_algorithm = public_key.algorithm self._uses_pkcs1 = self._key_algorithm in _PKCS1_ALGORITHMS + self._public_key_pem = public_key.pemThen update
get_public_key_pem()to use the cached value:def get_public_key_pem(self, **kwargs) -> bytes | str: encode = kwargs.get('encode', False) - - request = kms_v1.GetPublicKeyRequest( - name=self.key_name, - ) - public_key = self.kms_client.get_public_key(request=request) if encode: - return public_key.pem.encode('utf-8') - return public_key.pem + return self._public_key_pem.encode('utf-8') + return self._public_key_pemAlso applies to: 114-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wavefront/server/packages/flo_cloud/flo_cloud/gcp/kms.py` around lines 52 - 56, Cache the fetched public key PEM during initialization to avoid calling kms_client.get_public_key twice: when you call kms_client.get_public_key(request=kms_v1.GetPublicKeyRequest(name=self.key_name)) in the initializer, store both public_key.algorithm into self._key_algorithm and public_key.pem into a new attribute (e.g. self._public_key_pem) and set self._uses_pkcs1 accordingly; then update get_public_key_pem() to return the cached self._public_key_pem (with a safe fallback to call kms_client.get_public_key only if the cached value is missing) and keep verify() using get_public_key_pem() as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wavefront/server/scripts/test_kms_auth_flow.py`:
- Around line 107-111: The code prints the prefix check but continues even when
the token doesn't start with PREFIX; update the block around the print and
header_alg extraction to fail fast: after computing/reading token and PREFIX,
check token.startswith(PREFIX) and if false immediately exit with a non-zero
status or raise an exception (e.g., SystemExit or ValueError) so the script
stops instead of decoding the remainder; reference the variables token, PREFIX
and the header_alg extraction expression to locate where to insert the immediate
failure.
- Around line 35-41: The temporary private key file is created with
tempfile.NamedTemporaryFile(..., delete=False) and never removed, leaving secret
material on disk; modify the test_kms_auth_flow.py flow to either use
delete=True or explicitly remove the file after reading priv_pem (e.g., call
os.remove(priv.name) or use a context where the file is created without
delete=False), ensuring cleanup happens even on errors (use try/finally or
contextmanager) and reference the tempfile.NamedTemporaryFile invocation, the
priv variable, and the priv_pem read to locate where to add the removal.
- Line 37: The subprocess invocations that use the literal 'openssl' (e.g., the
call with args ['openssl', 'genrsa', '-out', priv.name, '2048'] and the similar
call later) should resolve the openssl executable to an absolute path at module
load using shutil.which('openssl') and fail fast if not found; replace the
string 'openssl' in those argument lists with the resolved path (and raise a
clear RuntimeError or SystemExit when shutil.which returns None) so subprocess
calls do not rely on ambient PATH and prevent path‑shadowing vulnerabilities.
---
Outside diff comments:
In `@wavefront/server/modules/auth_module/auth_module/services/token_service.py`:
- Line 30: Update the type annotation for the kms_service parameter to allow
None: change its annotation from FloKMS to FloKMS | None where it is declared
(the parameter named kms_service in token_service.py), so the signature matches
the runtime check `if kms_service is None` (and mirrors floconsole). Ensure any
internal references treat kms_service as optional or perform the existing None
check before use.
---
Nitpick comments:
In `@wavefront/server/packages/flo_cloud/flo_cloud/gcp/kms.py`:
- Around line 52-56: Cache the fetched public key PEM during initialization to
avoid calling kms_client.get_public_key twice: when you call
kms_client.get_public_key(request=kms_v1.GetPublicKeyRequest(name=self.key_name))
in the initializer, store both public_key.algorithm into self._key_algorithm and
public_key.pem into a new attribute (e.g. self._public_key_pem) and set
self._uses_pkcs1 accordingly; then update get_public_key_pem() to return the
cached self._public_key_pem (with a safe fallback to call
kms_client.get_public_key only if the cached value is missing) and keep verify()
using get_public_key_pem() as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f490dc96-c848-4993-bd07-368994d797a8
📒 Files selected for processing (5)
wavefront/server/apps/floconsole/floconsole/services/token_service.pywavefront/server/modules/auth_module/auth_module/services/token_service.pywavefront/server/packages/flo_cloud/flo_cloud/gcp/kms.pywavefront/server/packages/flo_cloud/flo_cloud/kms.pywavefront/server/scripts/test_kms_auth_flow.py
Comment on lines
+35
to
+41
| with tempfile.NamedTemporaryFile(suffix='.pem', delete=False) as priv: | ||
| subprocess.run( | ||
| ['openssl', 'genrsa', '-out', priv.name, '2048'], | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
| priv_pem = open(priv.name, 'rb').read() |
There was a problem hiding this comment.
Clean up temporary private key material after reading it.
delete=False leaves the generated private key file on disk and it is never removed. Even for a test script, this is an avoidable secret-leak risk.
Suggested fix
def _dummy_pem_keys() -> tuple[str, str]:
- with tempfile.NamedTemporaryFile(suffix='.pem', delete=False) as priv:
+ with tempfile.NamedTemporaryFile(suffix='.pem', delete=False) as priv:
+ priv_path = priv.name
subprocess.run(
- ['openssl', 'genrsa', '-out', priv.name, '2048'],
+ ['openssl', 'genrsa', '-out', priv_path, '2048'],
check=True,
capture_output=True,
)
- priv_pem = open(priv.name, 'rb').read()
+ try:
+ with open(priv_path, 'rb') as f:
+ priv_pem = f.read()
+ finally:
+ os.unlink(priv_path)🧰 Tools
🪛 Ruff (0.15.13)
[error] 36-36: subprocess call: check for execution of untrusted input
(S603)
[error] 37-37: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wavefront/server/scripts/test_kms_auth_flow.py` around lines 35 - 41, The
temporary private key file is created with tempfile.NamedTemporaryFile(...,
delete=False) and never removed, leaving secret material on disk; modify the
test_kms_auth_flow.py flow to either use delete=True or explicitly remove the
file after reading priv_pem (e.g., call os.remove(priv.name) or use a context
where the file is created without delete=False), ensuring cleanup happens even
on errors (use try/finally or contextmanager) and reference the
tempfile.NamedTemporaryFile invocation, the priv variable, and the priv_pem read
to locate where to add the removal.
| def _dummy_pem_keys() -> tuple[str, str]: | ||
| with tempfile.NamedTemporaryFile(suffix='.pem', delete=False) as priv: | ||
| subprocess.run( | ||
| ['openssl', 'genrsa', '-out', priv.name, '2048'], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Verify how openssl resolves in the current environment.
command -v openssl
python - <<'PY'
import shutil
p = shutil.which("openssl")
print(f"shutil.which('openssl') => {p}")
if not p:
raise SystemExit("openssl missing")
PYRepository: rootflo/wavefront
Length of output: 43
🏁 Script executed:
# First, locate and examine the test file
find . -name "test_kms_auth_flow.py" -type fRepository: rootflo/wavefront
Length of output: 110
🏁 Script executed:
cat -n ./wavefront/server/scripts/test_kms_auth_flow.pyRepository: rootflo/wavefront
Length of output: 5589
Resolve openssl to an absolute path before invoking subprocess.
Both calls to openssl on lines 37 and 43 use a partial executable path that relies on ambient PATH, creating a path shadowing vulnerability in compromised environments.
Use shutil.which() to resolve the absolute path at module load time and fail explicitly if openssl is not found:
Suggested fix
+import shutil
...
+OPENSSL_BIN = shutil.which('openssl')
+if not OPENSSL_BIN:
+ raise RuntimeError('openssl not found in PATH')
...
subprocess.run(
- ['openssl', 'genrsa', '-out', priv.name, '2048'],
+ [OPENSSL_BIN, 'genrsa', '-out', priv.name, '2048'],
check=True,
capture_output=True,
)
...
pub_proc = subprocess.run(
- ['openssl', 'rsa', '-pubout'],
+ [OPENSSL_BIN, 'rsa', '-pubout'],
input=priv_pem,
capture_output=True,
check=True,
)🧰 Tools
🪛 Ruff (0.15.13)
[error] 37-37: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wavefront/server/scripts/test_kms_auth_flow.py` at line 37, The subprocess
invocations that use the literal 'openssl' (e.g., the call with args ['openssl',
'genrsa', '-out', priv.name, '2048'] and the similar call later) should resolve
the openssl executable to an absolute path at module load using
shutil.which('openssl') and fail fast if not found; replace the string 'openssl'
in those argument lists with the resolved path (and raise a clear RuntimeError
or SystemExit when shutil.which returns None) so subprocess calls do not rely on
ambient PATH and prevent path‑shadowing vulnerabilities.
Comment on lines
+107
to
+111
| print(f' prefix ok={token.startswith(PREFIX)}') | ||
| header_alg = __import__('json').loads( | ||
| base64.urlsafe_b64decode(token[len(PREFIX) :].split('.')[0] + '==') | ||
| )['alg'] | ||
| print(f' JWT header alg={header_alg}') |
There was a problem hiding this comment.
Fail fast when token prefix validation fails.
The script currently prints prefix ok=False but still proceeds. That can hide regressions in prefix handling and produce misleading pass/fail outcomes.
Suggested fix
print(f' token length={len(token)}')
- print(f' prefix ok={token.startswith(PREFIX)}')
+ prefix_ok = token.startswith(PREFIX)
+ print(f' prefix ok={prefix_ok}')
+ if not prefix_ok:
+ print(' FAIL: token prefix mismatch')
+ return 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| print(f' prefix ok={token.startswith(PREFIX)}') | |
| header_alg = __import__('json').loads( | |
| base64.urlsafe_b64decode(token[len(PREFIX) :].split('.')[0] + '==') | |
| )['alg'] | |
| print(f' JWT header alg={header_alg}') | |
| prefix_ok = token.startswith(PREFIX) | |
| print(f' prefix ok={prefix_ok}') | |
| if not prefix_ok: | |
| print(' FAIL: token prefix mismatch') | |
| return 1 | |
| header_alg = __import__('json').loads( | |
| base64.urlsafe_b64decode(token[len(PREFIX) :].split('.')[0] + '==') | |
| )['alg'] | |
| print(f' JWT header alg={header_alg}') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wavefront/server/scripts/test_kms_auth_flow.py` around lines 107 - 111, The
code prints the prefix check but continues even when the token doesn't start
with PREFIX; update the block around the print and header_alg extraction to fail
fast: after computing/reading token and PREFIX, check token.startswith(PREFIX)
and if false immediately exit with a non-zero status or raise an exception
(e.g., SystemExit or ValueError) so the script stops instead of decoding the
remainder; reference the variables token, PREFIX and the header_alg extraction
expression to locate where to insert the immediate failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores