Implement AquaSec Authentication GitHub Action #5
Conversation
Co-authored-by: tmikula-dev <[email protected]>
Co-authored-by: tmikula-dev <[email protected]>
Co-authored-by: tmikula-dev <[email protected]>
Co-authored-by: tmikula-dev <[email protected]>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughThis PR implements a complete Python-based AquaSec Scan Results GitHub Action with authentication, input validation, and structured logging. It introduces an AquaSecAuthenticator class for API token generation, ActionInputs class for input validation, and a refactored action.yml to orchestrate the setup and execution flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant GHA as GitHub Action
participant Main as main.py
participant Input as ActionInputs
participant Auth as AquaSecAuthenticator
participant API as AquaSec API
User->>GHA: Trigger workflow with aqua-key, aqua-secret
GHA->>Main: Execute action (env vars set)
Main->>Main: setup_logging()
Main->>Input: ActionInputs().validate()
Input->>Input: get_aquasec_key()
Input->>Input: get_aquasec_secret()
Input-->>Main: validation result (bool)
alt Validation Success
Main->>Auth: AquaSecAuthenticator().authenticate()
Auth->>Auth: _generate_signature(request)
Auth->>API: POST /v2/tokens (signed request)
API-->>Auth: {bearer_token}
Auth-->>Main: bearer_token
Main->>Main: Log success & continue
else Validation or Auth Failure
Main->>Main: Log error & exit(1)
Main-->>User: Exit code 1
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related Issues
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
tmikula-dev
added
enhancement
|
@copilot review |
…earer token Co-authored-by: tmikula-dev <[email protected]>
Code review completed. Fixed critical bug where bearer token was not being set as GitHub Actions output. Changes made (commit f7e6000):
Quality checks - all passing:
The action now properly outputs the bearer token for use in downstream workflow steps. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@action.yml`:
- Around line 28-33: The "Prepare environment variables" step currently echoes
raw inputs (INPUT_AQUA_KEY, INPUT_AQUA_SECRET, INPUT_VERBOSE_LOGGING) into
$GITHUB_ENV which can leak secrets into logs; before writing each sensitive
input (at least inputs.aqua-key and inputs.aqua-secret) call the GitHub Actions
masking command (::add-mask::${{ inputs.aqua-key }} and ::add-mask::${{
inputs.aqua-secret }}) so the values are redacted in logs, then write them to
$GITHUB_ENV as before; update the step named "Prepare environment variables" to
emit the add-mask lines for the two secrets prior to echoing into $GITHUB_ENV.
In `@main.py`:
- Around line 44-48: The bearer_token returned by
AquaSecAuthenticator().authenticate() is never written to GitHub Actions
outputs; after successful assignment to bearer_token in the try block, write the
token to GITHUB_OUTPUT (append a line like "bearer-token=<token>" to the file at
os.environ['GITHUB_OUTPUT']) so it is available as
steps.auth.outputs.bearer-token, and in the except block replace logger.error
with logger.exception to log the stack trace when catching (ValueError,
RequestException) as e; reference AquaSecAuthenticator.authenticate, the
bearer_token variable, and the except block surrounding the authenticate call
when making these changes.
In `@src/model/authenticator.py`:
- Around line 98-104: Wrap the call to response.json() in a try/except to catch
JSON parsing errors and unexpected response shape: call response.json() inside a
try block, catch json.JSONDecodeError (or ValueError for compatibility) and
raise a clear ValueError including response.status_code and response.text; then
validate the parsed payload contains the expected "data" key and non-empty
bearer_token (the variable bearer_token) and raise a ValueError with context if
missing. This change should be applied around the existing response.json() usage
(where bearer_token is assigned) to ensure malformed JSON or missing keys are
handled gracefully.
In `@tests/utils/test_logging_config.py`:
- Around line 60-75: The tests test_setup_logging_verbose_logging_enabled and
test_setup_logging_debug_mode_enabled_by_ci set environment variables but do not
restore them; update these tests to use pytest's monkeypatch fixture (e.g.,
monkeypatch.setenv("INPUT_VERBOSE_LOGGING", "true") and
monkeypatch.setenv("RUNNER_DEBUG", "1")) or explicitly save and restore
os.environ around the calls to setup_logging so environment state is isolated;
keep the rest of the test logic (caplog, setup_logging, validate_logging_config,
mock_logging_setup) unchanged.
🧹 Nitpick comments (7)
.github/copilot-instructions.md (1)
33-33: Assert pattern differs from common pytest convention.The documented pattern
assert expected == actualreverses the typical pytest convention ofassert actual == expected. While functionally equivalent, the conventional order improves assertion error readability (e.g., "AssertionError: assert 5 == 10" reads as "actual was 5, expected 10").Consider aligning with the common convention or explicitly noting this is a project-specific choice.
src/utils/utils.py (1)
24-24: Unused logger import.The
loggeris initialized but not used anywhere in this module. Consider removing it to keep the code clean, or add a comment if it's intended for future use.♻️ Suggested fix
-import logging import os - -logger = logging.getLogger(__name__)tests/test_main.py (1)
30-36: Consider adding assertions for the successful path.The test verifies that
run()completes without raising an exception, but doesn't assert that the expected methods were called or that the token was processed. Adding assertions would make the test more robust.♻️ Suggested improvement
def test_run_successful(mocker): mocker.patch("main.setup_logging") - mocker.patch("main.ActionInputs.validate", return_value=True) - mocker.patch("main.AquaSecAuthenticator.authenticate", return_value="test_token") + mock_validate = mocker.patch("main.ActionInputs.validate", return_value=True) + mock_authenticate = mocker.patch("main.AquaSecAuthenticator.authenticate", return_value="test_token") run() + + mock_validate.assert_called_once() + mock_authenticate.assert_called_once()tests/model/test_authenticator.py (1)
29-36: Consider adding a deterministic signature verification test.The current test only verifies the output is a 64-character hex string. Adding a test with known input/output would verify the HMAC-SHA256 implementation correctness.
♻️ Suggested additional test
def test_generate_signature_returns_correct_hmac(): import hmac import hashlib authenticator = AquaSecAuthenticator() authenticator.api_secret = "known_secret" test_input = "known_input" actual = authenticator._generate_signature(test_input) expected = hmac.new( b"known_secret", b"known_input", hashlib.sha256 ).hexdigest() assert actual == expectedRegarding the static analysis hint (S105): This is a false positive—using a hardcoded test secret in unit tests is appropriate and expected.
tests/utils/test_utils.py (1)
36-42: Consider adding a test for the default value parameter.The
get_action_inputfunction accepts adefaultparameter that isn't explicitly tested here.♻️ Suggested additional test
def test_get_input_returns_default_when_env_var_not_set(mocker): mocker.patch("os.getenv", return_value=None) actual = get_action_input("missing-input", default="fallback_value") assert actual is None # os.getenv returns None, not the default def test_get_input_uses_custom_default(mocker): mock_getenv = mocker.patch("os.getenv", return_value="custom_default") actual = get_action_input("some-input", default="custom_default") mock_getenv.assert_called_with("INPUT_SOME_INPUT", default="custom_default") assert "custom_default" == actualsrc/action_inputs.py (1)
56-80: Consider adding secret masking for GitHub Actions logs.The PR description mentions "automatic secret masking via
::add-mask::" but this isn't implemented in the validate method. Without masking, if secrets are accidentally logged elsewhere, they could be exposed in the workflow logs.Proposed addition for secret masking
def validate(self): """ Validates the action inputs. Returns: True if all required inputs are valid, False otherwise. """ logger.info("AquaSec Scan Results - Input validation starting.") error_count: int = 0 aquasec_key: str = self.get_aquasec_key() aquasec_secret: str = self.get_aquasec_secret() + # Mask secrets in GitHub Actions logs + if aquasec_key: + print(f"::add-mask::{aquasec_key}") + if aquasec_secret: + print(f"::add-mask::{aquasec_secret}") + if not aquasec_key or not isinstance(aquasec_key, str): logger.error("AQUASEC_KEY: str - not provided.") error_count += 1src/model/authenticator.py (1)
73-84: Consider reusing ActionInputs for credential retrieval.The credentials are fetched directly via
get_action_input()here, duplicating the logic inActionInputs.get_aquasec_key()andget_aquasec_secret(). Consider usingActionInputsfor consistency, though this is a minor organizational concern.
…output bearer token" This reverts commit f7e6000.
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
This task is ready to review.
Implementation Complete ✅
All acceptance criteria have been met for the AquaSec Authentication GitHub Action.
Release Notes
Summary of Changes
action.ymlwith inputs (aqua-key, aqua-secret) and output (bearer-token)src/action_inputs.pyfor input loading and validationsrc/model/authenticator.pywith AquaSec authentication logicmain.pyto implement complete entry point with loggingQuality Metrics
Latest Fix
set_action_outputfunction tosrc/utils/utils.pyto properly output the bearer token as a GitHub Actions outputmain.pyto callset_action_outputwith the bearer tokenbearer_tokenvariable💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.