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

feat(parser): support ssl certificates and swagger files set as attributes for Ansible and Terraform #2958 #2960

Merged
merged 10 commits into from
May 4, 2021
Merged

feat(parser): support ssl certificates and swagger files set as attributes for Ansible and Terraform #2958 #2960

merged 10 commits into from
May 4, 2021

Conversation

cosmicgirl97
Copy link
Contributor

@cosmicgirl97 cosmicgirl97 commented Apr 22, 2021

Closes #2958

Proposed Changes

  • The algorithm followed to access the content of the attribute 'swagger_file' in an Ansible template was:
    1. Verify if any attribute in the Ansible template is 'swagger_file' in the function 'playbookParser' (\kics\pkg\parser\yaml\parser.go);
    2. In a positive case, add the content of the swagger file in the attribute 'swagger_file' (through function 'AddSwaggerInfo' used in the function 'playbookParser');

    Functions description:

    • AddSwaggerInfo (\kics\pkg\parser\additional\swagger_utils.go): Firstly, checks if the swagger path (defined in 'swagger_file') is not a full valid path or is an incomplete path. In case of true, joins the current dir with the incomplete path. Then, read the file and add the content;
  • The algorithm followed to access the content of the attribute 'certificate' in an Ansible template and the attribute 'certificate_body' in Terraform was:

    1. For Ansible, verify if any attribute in the Ansible template is 'certificate' in the function 'playbookParser' (\kics\pkg\parser\yaml\parser.go); For Terraform, verify if any attribute in the Terraform template is 'certificate_body' and refers a pem file in the function 'Parse' (\kics\pkg\parser\terraform\terraform.go);
    2. In a positive case, add the content of the certificate (through function 'AddCertificateInfo').

    Functions description:

    • AddCertificateInfo (kics\pkg\parser\additional\certificate_utils.go): Firstly, checks if the certificate path (defined in the attribute related to certificate) is not a full valid path or is an incomplete path. In case of true, joins the current dir with the incomplete path. Then, read the file and add the information about the expiration date and the RSA Key bytes (through function 'getCertificateInfo')
    • getCertificateInfo (kics\pkg\parser\additional\certificate_utils.go): reads and parses the certificate PEM and gets the information about the expiration date (cert.NotAfter) and the RSA Key bytes (cert.PublicKey.(*rsa.PublicKey).Size())

Analysis of consumption metrics (still work in progress)

The following table is related to the scan of "\kics\assets\queries\ansible\aws". For that, it needed to add a YAML file in queries ''API Gateway Endpoint Config is Not Private", "API Gateway Without SSL Certificate" and "API Gateway X-Ray Disabled" (the positive and negative files refer to a swagger_file that does not exist in the respective folder). In these two rounds, the results of metrics 'Total CPU usage for inspect', 'Total CPU usage for generate_report' and 'Total MEM usage for get_sources' are inconclusive.

Without Parser Changes R1 With Parser Changes R1 Difference R1 Without Parser Changes R2 With Parser Changes R2 Difference R2
Total CPU usage for get_queries 13.11s 14.79s +1.68s 13.23s 17.59s +4.36s
Total CPU usage for get_sources 3.74s 2.71s -1.03s 3.34s 1.29s -2.05s
Total CPU usage for inspect 54.23s 56.95s +2.72s 56.70s 45.96s -10.74s
Total CPU usage for generate_report 680.00ms 1.13s +0.45s 1.23s 630.00ms -0.6s
Total MEM usage for get_queries 241.73MB 260.29MB +18.56MB 251.83MB 261.15MB +9.32MB
Total MEM usage for get_sources 344.13MB 311.62MB -32.51MB 301.76MB 314.21MB +12.45MB
Total MEM usage for inspect 358.83MB 357.80MB -1.03MB 363.87MB 357.71MB -6.16MB
Total MEM usage for generate_report 360.03MB 357.80MB -2.23MB 363.87MB 357.71MB -6.16MB

Queries:

  • Added "Certificate RSA Key Bytes Lower Than 128" for Ansible and Terraform (AWS): It checks if a certificate uses a RSA key with length higher than 128 bytes
  • Added "API Gateway Without Configured Authorizer" for Ansible (AWS). It checks if a swagger attribute has a authorizer set

I submit this contribution under the Apache-2.0 license.

@cosmicgirl97 cosmicgirl97 marked this pull request as draft April 22, 2021 15:19
@rogeriopeixotocx rogeriopeixotocx changed the title Add support to certificate and swagger files set as attributes in Ansible and Terraform Closes#2958 Add support to certificate and swagger files set as attributes in Ansible and Terraform #2958 Apr 22, 2021
@rogeriopeixotocx rogeriopeixotocx added community Community contribution feature New feature go Pull requests that update Go code help wanted Extra attention is needed labels Apr 22, 2021
@rogeriopeixotocx rogeriopeixotocx added this to the Core Engineering milestone Apr 22, 2021
@cosmicgirl97 cosmicgirl97 reopened this Apr 22, 2021
@rogeriopeixotocx rogeriopeixotocx changed the title Add support to certificate and swagger files set as attributes in Ansible and Terraform #2958 feat(parser): support ssl certificates and swagger files set as attributes for Ansible and Terraform #2958 Apr 23, 2021
Copy link
Contributor

@rogeriopeixotocx rogeriopeixotocx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good stuff. But it still needs work.

@rogeriopeixotocx
Copy link
Contributor

Also @cosmicgirl97 (AKA @rafaela-soares) We need unit tests 😄

@rogeriopeixotocx
Copy link
Contributor

@cosmicgirl97 @rafaela-soares do you have a query to add that uses the certificate info appended to the payload? It would be great to see this new feature in action being used by a query in this same PR

Copy link
Contributor

@felipe-avelar felipe-avelar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, check comments

@rogeriopeixotocx
Copy link
Contributor

@cosmicgirl97 @rafaela-soares please add the new terraform samples e.g: assets/queries/terraform/aws/certificate_rsa_key_bytes_lower_than_128/test/negative2.tf to the ignore list until we fix this.

Copy link
Collaborator

@cx-joao-reigota cx-joao-reigota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Won't affect, concurrent Scans PR since the changes are made in the parser

@cosmicgirl97 cosmicgirl97 marked this pull request as ready for review April 30, 2021 16:15
Copy link
Contributor

@rogeriopeixotocx rogeriopeixotocx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rogeriopeixotocx rogeriopeixotocx dismissed stale reviews from cx-joao-reigota and felipe-avelar May 4, 2021 10:30

outdated

@rogeriopeixotocx rogeriopeixotocx merged commit 6fae892 into Checkmarx:master May 4, 2021
@kaplanlior kaplanlior removed the community Community contribution label Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature go Pull requests that update Go code help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to certificate and swagger files set as attributes in Ansible and Terraform
5 participants