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

fix: DecodeCertificates cert length check #1470

Merged
merged 7 commits into from
May 14, 2024

Conversation

susanshi
Copy link
Collaborator

Description

What this PR does / why we need it:

DecodeCertificates should throw error when no certs found

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes # 1459

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Validated when KeyManagementProvider if contentType does not matches content value, Status is false with expected error msg:

kubectl describe output below for Keys:

MBkGA1UEAxMSd2FiYml0LW5ldHdvcmtzLmlvMCAXDTIyMTIwMjA4MDg0NFoYDzIx
MjIxMjAzMDgwODQ0WjBaMQswCQYDVQQGEwJVUzELMAkGA1UECBMCV0ExEDAOBgNV
BAcTB1NlYXR0bGUxDzANBgNVBAoTBk5vdGFyeTEbMBkGA1UEAxMSd2FiYml0LW5l
dHdvcmtzLmlvMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnoskJWB0
ZsYcfbTvCYQMLqWaB/yN3Jf7Ryxvndrij83fWEQPBQJi8Mk8SpNqm2x9uP3gsQDc
L/73a0p6/D+hza2jQQVhebe/oB0LJtUoD5LXlJ83UQdZETLMYAzeBNcBR4kMecrY
CnE6yjHeiEWdAH+U7Mt39zJh+9lGIcbk0aUE5UOp8o3t5RWFDcl9hQ7QOXROwmpO
thLUIiY/bcPpsg/2nH1nzFjqiBef3sgopFCTgtJ7qF8B83Xy/+hJ5vD29xsbSwuB
3iLE7qLxu2NxdIa4oL0Y2QKMh/getjI0xnvwAmPkFiFbzC7LFdDfd6+gA5GpUXxL
u6UmwucAgiljGQIDAQABoycwJTAOBgNVHQ8BAf8EBAMCB4AwEwYDVR0lBAwwCgYI
KwYBBQUHAwMwDQYJKoZIhvcNAQELBQADggEBAFvRW/mGjnnMNFKJc/e3o/+yiJor
dcrq/1UzyD7eNmOaASXz8rrrFT/6/TBXExPuB2OIf9OgRJFfPGLxmzCwVgaWQbK0
VfTN4MQzRrSwPmNYsBAAwLxXbarYlMbm4DEmdJGyVikq08T2dZI51GC/YXEwzlnv
ldN0dBflb/FKkY5rAp0JgpHLGKeStxFvB62noBjWfrm7ShCf9gkn1CjmgvP/sYK0
pJgA1FHPd6EeB6yRBpLV4EJgQYUJoOpbHz+us62jKj5fAXsX052LPmk9ArmP0uJ1
CJLNdj+aShCs4paSWOObDmIyXHwCx3MxCvYsFk/Wsnwura6jGC+cNsjzSx4=
-----END CERTIFICATE-----

Type: inline
Status:
Brieferror: failed to create key managemen...
Error: failed to create key management provider provider: Error: key invalid, Code: KEY_INVALID, Component Type: keyManagementProvider
Issuccess: false
Lastfetchedtime: 2024-05-13T05:58:51Z
Events:

kubectl describe output below for Certificates:

Content Type:  certificate
Value:         -----BEGIN PUBLIC KEY-----

MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAweAc4xikYT4ZszXVdF5m
rgP0zKVYi4Ces0py9dw8XZfh/Hlxb5xWMs4DzTcKwmLatgKNSrvNyOaxkBD90Pvc
YNaTCwzwQ09kZ5dYtVOV4sdzeyOj8UDtf4MF5eJgJj/wWCQJnWrX/4n6nSdNTXSJ
EFAZkDv0BKVkZekJHn3fh+pOuv8UtvOrY1NjNK/TLWxB+8xpwugeB9oZ+VgV/gHZ
BLprxYkmUDsfngYy3+r6RZ+hInalZc5uAbtRUoB8+nVhXXOe3iVcVWFoWPMJ2fuP
Hz/8cDjv02MNWa/MeAt+ItW3N+VFZNkwbu5en3FepsxzRl04rhZzr1DSX6V6CVX4
3wIDAQAB
-----END PUBLIC KEY-----

Type: inline
Status:
Brieferror: failed to create key managemen...
Error: failed to create key management provider provider: Error: cert invalid, Code: CERT_INVALID, Component Type: keyManagementProvider
Issuccess: false
Lastfetchedtime: 2024-05-13T05:58:10Z
Events:

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

Post Merge Requirements

  • MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 68.18%. Comparing base (1bd347c) to head (bf4b59e).
Report is 24 commits behind head on dev.

Files Patch % Lines
pkg/keymanagementprovider/inline/provider.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1470      +/-   ##
==========================================
+ Coverage   66.76%   68.18%   +1.41%     
==========================================
  Files         116      119       +3     
  Lines        6030     6138     +108     
==========================================
+ Hits         4026     4185     +159     
+ Misses       1620     1559      -61     
- Partials      384      394      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@susanshi
Copy link
Collaborator Author

@binbin-li @akashsinghal , looks like its easier to return the err. The error already contains the correct err code and component type. See output:

image

image

Copy link
Collaborator

@binbin-li binbin-li left a comment

Choose a reason for hiding this comment

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

lgtm

@susanshi susanshi merged commit 68c93a6 into ratify-project:dev May 14, 2024
17 checks passed
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.

None yet

3 participants