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

add seccomp in-cluster tests #394

Merged
merged 1 commit into from
Jun 25, 2024
Merged

add seccomp in-cluster tests #394

merged 1 commit into from
Jun 25, 2024

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Jun 19, 2024

PR Type

Enhancement, Tests


Description

  • Added SeccompProfileTests to the system tests, including pod and container level tests.
  • Created Kubernetes deployment configurations for nginx with seccomp profiles.
  • Enhanced error logging and formatting in systest-cli.py.
  • Added method to retrieve application profiles from storage in base_k8s.py.
  • Updated system test mapping to include seccomp profile tests.

Changes walkthrough 📝

Relevant files
Enhancement
5 files
tests.py
Add SeccompProfileTests to system tests.                                 

configurations/system/tests.py

  • Added import for SeccompProfileTests.
  • Included SeccompProfileTests in all_tests_names and get_test.
  • Reordered imports for consistency.
  • +7/-5     
    kubectl_wrapper.py
    Include SeccompProfile in kubectl wrapper resources.         

    infrastructure/kubectl_wrapper.py

    • Added SeccompProfile to the list of known resources.
    +1/-0     
    systest-cli.py
    Enhance error logging and formatting in systest-cli.         

    systest-cli.py

  • Improved error logging for test validation and customer checks.
  • Minor formatting adjustments.
  • +15/-8   
    statics.py
    Add seccomp path and application profile constants.           

    systest_utils/statics.py

  • Added DEFAULT_SECCOMP_PATH for seccomp tests.
  • Included APPLICATION_PROFILE_PLURAL in known server constants.
  • +4/-0     
    base_k8s.py
    Implement method to get application profiles from storage.

    tests_scripts/kubernetes/base_k8s.py

    • Added method to retrieve application profiles from storage.
    +24/-0   
    Tests
    6 files
    seccomp_tests.py
    Implement SeccompProfileTests with pod and container level tests.

    configurations/system/tests_cases/seccomp_tests.py

  • Created SeccompProfileTests class.
  • Added static methods for pod and container level seccomp profile
    tests.
  • +31/-0   
    seccomp.py
    Add SeccompProfile class for Helm and Kubescape integration.

    tests_scripts/helm/seccomp.py

  • Created SeccompProfile class for Helm and Kubescape integration.
  • Implemented methods for Helm chart installation and seccomp profile
    application.
  • +75/-0   
    nginx-seccomp-container.yaml
    Add nginx deployment with container level seccomp profile.

    configurations/k8s_workloads/seccomp/nginx-seccomp-container.yaml

  • Added Kubernetes deployment configuration for nginx with container
    level seccomp profile.
  • +20/-0   
    nginx-seccomp-pod.yaml
    Add nginx deployment with pod level seccomp profile.         

    configurations/k8s_workloads/seccomp/nginx-seccomp-pod.yaml

  • Added Kubernetes deployment configuration for nginx with pod level
    seccomp profile.
  • +20/-0   
    seccomp-nginx.yaml
    Add seccomp profile configuration for nginx.                         

    configurations/k8s_workloads/seccomp/seccomp-nginx.yaml

    • Added seccomp profile configuration for nginx.
    +137/-0 
    system_test_mapping.json
    Update system test mapping with seccomp profile tests.     

    system_test_mapping.json

  • Added entries for seccomp_profile_pod and seccomp_profile_container
    tests.
  • +1470/-1436

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Signed-off-by: Matthias Bertschy <[email protected]>
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 4
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The PR introduces new tests for seccomp profiles, but it's unclear if the tests handle exceptions or edge cases related to seccomp profile failures or misconfigurations. It would be beneficial to include negative test cases or error handling scenarios.
    Code Organization:
    The PR adds new classes and methods which are well-organized, but the separation of concerns could be improved. For instance, the SeccompProfileTests class in seccomp_tests.py could be split into more granular classes or modules based on functionality to enhance maintainability.
    Dependency Management:
    The PR includes changes in systest-cli.py and other scripts that import new modules. It's important to ensure that these dependencies are properly managed and compatible with the existing system to avoid runtime issues.

    Copy link

    Failed to generate code suggestions for PR

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Apply more restrictive actions to critical syscalls in the Seccomp profile

    To enhance the security posture, consider specifying more granular actions for certain
    syscalls instead of a blanket SCMP_ACT_ALLOW. For example, critical syscalls like execve,
    clone, and mount could be set to SCMP_ACT_ERRNO to prevent their usage unless explicitly
    needed.

    configurations/k8s_workloads/seccomp/seccomp-nginx.yaml [88-118]

     - execve
    +  action: SCMP_ACT_ERRNO
     - clone
    +  action: SCMP_ACT_ERRNO
     - mount
    -action: SCMP_ACT_ALLOW
    +  action: SCMP_ACT_ERRNO
     
    Suggestion importance[1-10]: 10

    Why: Specifying more granular actions for critical syscalls like execve, clone, and mount significantly enhances security by preventing their usage unless explicitly needed.

    10
    Change the default action for unspecified syscalls to log instead of error

    To ensure better security and maintainability, consider specifying a defaultAction of
    SCMP_ACT_LOG for syscalls not explicitly allowed or denied. This will log all
    non-specified syscalls without stopping them, which can be useful for debugging and
    understanding application behavior without compromising security.

    configurations/k8s_workloads/seccomp/seccomp-nginx.yaml [10]

    -defaultAction: SCMP_ACT_ERRNO
    +defaultAction: SCMP_ACT_LOG
     
    Suggestion importance[1-10]: 7

    Why: Changing the default action to log can help in debugging and understanding application behavior without stopping the application. However, it may not be suitable for all environments, especially where security is a higher priority than logging.

    7
    Maintainability
    Remove duplicate syscalls from the Seccomp profile

    Remove duplicate syscall entries to clean up the Seccomp profile and avoid potential
    confusion. For instance, mmap, futex, setsockopt, epoll_wait, and getsockname are listed
    more than once.

    configurations/k8s_workloads/seccomp/seccomp-nginx.yaml [20-136]

    +- mmap
    +- futex
    +- setsockopt
    +- epoll_wait
    +- getsockname
     
    -
    Suggestion importance[1-10]: 9

    Why: Removing duplicate syscalls improves the maintainability and readability of the Seccomp profile, reducing potential confusion and errors.

    9
    Replace hard-coded JSON file path with a constant for better maintainability

    Instead of using a hard-coded string for the JSON file path in the validate_test function,
    consider using a constant or a configuration setting. This will make the code more
    maintainable and flexible if the path needs to change in the future.

    systest-cli.py [26]

    -Logger.logger.info(f"Test '{test_name}' is defined in ['{json_file_path}'] JSON.")
    +JSON_FILE_PATH = 'system_test_mapping.json'
    +Logger.logger.info(f"Test '{test_name}' is defined in ['{JSON_FILE_PATH}'] JSON.")
     
    Suggestion importance[1-10]: 8

    Why: Using a constant for the JSON file path improves maintainability and flexibility, making it easier to update the path in the future if needed. This is a good practice for configuration settings.

    8
    Group syscalls in the Seccomp profile for better readability and maintainability

    Consider grouping syscalls by categories or functionalities to improve the readability and
    maintainability of the Seccomp profile. This can also help in managing large lists and
    making it easier to update or audit.

    configurations/k8s_workloads/seccomp/seccomp-nginx.yaml [16-136]

    +# Network-related syscalls
     - accept4
    -- epoll_wait
    -- pselect6
    +- getsockname
    +- connect
     ...
    +# File system operations
    +- openat
     - fchown
    -- getsockname
    +- unlinkat
     
    Suggestion importance[1-10]: 8

    Why: Grouping syscalls by categories or functionalities enhances readability and maintainability, making it easier to manage and audit the profile.

    8
    Consolidate error handling in the main function to reduce code duplication

    Refactor the error handling in the main function to consolidate the exit conditions into a
    single block to avoid code duplication and improve readability.

    systest-cli.py [131-139]

    -if args.test_name not in ALL_TESTS:
    +if args.test_name not in ALL_TESTS or args.customer != CREDENTIALS.customer:
         Logger.logger.error(
    -        f"Test '{args.test_name}' is not defined in the system tests. Please add the test and try again.")
    +        f"Test '{args.test_name}' is not defined in the system tests or customer '{args.customer}' is different from '{CREDENTIALS.customer}'.")
         print_configurations()
         exit(1)
     
    Suggestion importance[1-10]: 7

    Why: Consolidating the error handling reduces code duplication and improves readability. However, it slightly reduces the granularity of error messages, which might be useful in some debugging scenarios.

    7
    Specify conditions under which tests should be skipped

    For better control and understanding of when tests should be skipped, provide explicit
    conditions or environments in the 'skip_on_environment' field instead of leaving it empty.

    system_test_mapping.json [8]

    -"skip_on_environment": ""
    +"skip_on_environment": "specify-environment-or-condition"
     
    Suggestion importance[1-10]: 7

    Why: Specifying conditions for skipping tests enhances maintainability and clarity, but it is not as critical as ensuring ownership or meaningful descriptions. It is a good practice for better control over test execution.

    7
    Possible bug
    Add a check to prevent IndexError when accessing elements in an empty list

    Add a check to ensure that the applicationProfiles list is not empty before accessing its
    elements to avoid potential IndexError.

    tests_scripts/kubernetes/base_k8s.py [71]

    +if not applicationProfiles:
    +    raise ValueError("No application profiles found.")
     path = applicationProfiles[0]['spec']['containers'][0]["seccompProfile"]["path"]
     
    Suggestion importance[1-10]: 9

    Why: Adding a check to ensure the list is not empty before accessing its elements prevents potential IndexError, which is crucial for avoiding runtime errors and improving code reliability.

    9
    Best practice
    Ensure all test configurations have an owner specified

    To avoid potential configuration errors, ensure that the 'owner' field is populated for
    all test configurations. This field is crucial for accountability and contact purposes in
    case of issues or required updates.

    system_test_mapping.json [27]

    -"owner": ""
    +"owner": "[email protected]"
     
    Suggestion importance[1-10]: 9

    Why: Ensuring the 'owner' field is populated is crucial for accountability and maintenance. This is a best practice that helps in identifying responsible parties for each test configuration, which is important for operational efficiency.

    9
    Enhancement
    Add meaningful descriptions to the test configurations

    It is recommended to provide meaningful descriptions for each test configuration to help
    understand the purpose and scope of the tests. This will aid in maintenance and clarity
    for new developers or for audit purposes.

    system_test_mapping.json [7]

    -"description": ""
    +"description": "Provide a meaningful description here."
     
    Suggestion importance[1-10]: 8

    Why: Adding meaningful descriptions improves code maintainability and clarity, which is beneficial for new developers and for auditing purposes. This is a significant enhancement for documentation and understanding.

    8
    Possible issue
    Verify and populate the target repositories for each test configuration

    To ensure that the test configurations are targeting the correct repositories, verify and
    populate the 'target_repositories' field where it is currently empty. This prevents the
    tests from being inadvertently skipped or misconfigured.

    system_test_mapping.json [6]

    -"target_repositories": []
    +"target_repositories": ["repository1", "repository2"]
     
    Suggestion importance[1-10]: 8

    Why: Populating the 'target_repositories' field is important to ensure tests are correctly configured and not inadvertently skipped. This is a significant improvement for preventing potential configuration issues.

    8
    Robustness
    Add error handling for dictionary update to ensure robustness

    Consider adding error handling for the update method call on helm_kwargs to handle
    potential exceptions when updating the dictionary, ensuring robustness.

    tests_scripts/helm/seccomp.py [24]

    -self.helm_kwargs.update(test_helm_kwargs)
    +try:
    +    self.helm_kwargs.update(test_helm_kwargs)
    +except Exception as e:
    +    Logger.logger.error(f"Failed to update helm_kwargs: {str(e)}")
    +    raise
     
    Suggestion importance[1-10]: 6

    Why: Adding error handling for the dictionary update improves robustness by catching potential exceptions. However, the likelihood of an exception occurring in this context is relatively low.

    6

    @matthyx
    Copy link
    Contributor Author

    matthyx commented Jun 19, 2024

    to be used with the argument --kwargs helm_branch=seccomp

    @matthyx matthyx merged commit 17e2db9 into master Jun 25, 2024
    3 checks passed
    @matthyx matthyx deleted the seccomp branch June 25, 2024 05:11
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants