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(extra730): Handle invalid date formats checking ACM certificates #1033

Merged

Conversation

jfagoagas
Copy link
Member

Context

extra730 checks if ACM Certificates are about to expire in $DAYS_TO_EXPIRE_THRESHOLD days or less

Description

This PR addresses the following:

  • Modify bsd_timestamp_to_date and gnu_timestamp_to_date in order to perform date validation properly.
  • Handle timestamp errors when an ACM certificate has not expiration date.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jfagoagas jfagoagas requested a review from a team as a code owner February 7, 2022 10:09
Comment on lines -37 to -39
if [[ $1 = 20* ]];then
echo $1 | cut -f1 -d"T"
else
Copy link
Member Author

Choose a reason for hiding this comment

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

I've deleted this date parsing because it allows something like 20wathever

@jfagoagas jfagoagas added the status/waiting-for-revision Waiting for maintainer's revision label Feb 8, 2022
# "Check if ACM Certificates are about to expire in $DAYS_TO_EXPIRE_THRESHOLD days or less"
for regx in $REGIONS; do
LIST_OF_ACM_CERTS=$($AWSCLI acm list-certificates $PROFILE_OPT --region $regx --query 'CertificateSummaryList[].CertificateArn' --output text)
if [[ $LIST_OF_ACM_CERTS ]];then
LIST_OF_ACM_CERTS=$("${AWSCLI}" acm list-certificates ${PROFILE_OPT} --region "${regx}" --include keyTypes="${ACM_KEY_TYPES}" --certificate-statuses "${ACM_CERTIFICATE_STATUSES}" --query 'CertificateSummaryList[].CertificateArn' --output text)
Copy link
Member

Choose a reason for hiding this comment

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

What about imported certificates?

Copy link
Member Author

Choose a reason for hiding this comment

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

It checks every ACM certificate whether it is Amazon Issued, Imported or comes from a Private PKI.

For that reason we have included two filters:

  • ACM_KEY_TYPES: to set the algorithms that can be used to generate key pairs.
  • ACM_CERTIFICATE_STATUSES: to only check ACM Certificates whose status is ISSUED. The following statuses are not checked:
PENDING_VALIDATION
INACTIVE
EXPIRED
VALIDATION_TIMED_OUT
REVOKED
FAILED

Comment on lines -49 to -51
if [[ $1 = 20* ]];then
echo $1 | cut -f1 -d"T"
else
Copy link
Member Author

Choose a reason for hiding this comment

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

I've deleted this date parsing because it allows something like 20wathever

# remove fractions of a second
TIMESTAMP_TO_CONVERT=$(cut -f1 -d"." <<< "${1}")
OUTPUT_DATE=$("${DATE_CMD}" -d @"${TIMESTAMP_TO_CONVERT}" +'%Y-%m-%d')
echo "${OUTPUT_DATE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this echo really needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's the way to return the OUTPUT_DATE value

@n4ch04 n4ch04 self-requested a review February 9, 2022 08:51
@jfagoagas jfagoagas merged commit 5c6902b into master Feb 9, 2022
@jfagoagas jfagoagas deleted the PRWLR-15-bug-extra-730-acm-certificates-date-validation branch February 9, 2022 16:56
@jfagoagas jfagoagas removed the status/waiting-for-revision Waiting for maintainer's revision label Feb 9, 2022
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