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

p11_child: enable more than one CRL PEM file #6104

Closed
wants to merge 1 commit into from

Conversation

ikerexxe
Copy link
Contributor

@ikerexxe ikerexxe commented Apr 8, 2022

Enable support for more than one CRL PEM file. p11_child parses the
crl_file list passed as argument, loads all the files and makes the
validation.

Finally, add a new test case in test_utils to check that the p11_child
crl_file argument has been parsed correctly. Add another three test
cases in test_oam_srv to check the validation process.

Resolves: #6086

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Cppcheck Result

You're Tops!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Cppcheck Result

Out Of Sight!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Cppcheck Result

You Rock!

@ikerexxe
Copy link
Contributor Author

The check failures aren't related with the changes so this PR is ready for review.

@sumit-bose
Copy link
Contributor

Hi,

thanks for the patch, the actual code looks good. I think I would have used a list instead of calling talloc_realloc() but this has some overhead as well and since I would expect only a few CRLs (<10) will be used in the configuration talloc_realloc() is ok imo.

Please see my in-line comment about the tests.

bye,
Sumit

@sumit-bose
Copy link
Contributor

Hi,

thanks, tests are looking good. I've added two inline comments and I'd like to ask you to add a :relnote: or :config: to the commit message.

bye,
Sumit

Enable support for more than one CRL PEM file. p11_child parses the
crl_file list passed as argument, loads all the files and makes the
validation.

Finally, add a new test case in test_utils to check that the p11_child
crl_file argument has been parsed correctly. Add another five test
cases in test_oam_srv to check the validation process.

:config: multiple crl_file arguments can be used in the
certificate_verification option.

Resolves: SSSD#6086

Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
@ikerexxe
Copy link
Contributor Author

I've added a :config: section in the commit and it's ready for review again.

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thanks, I'm fine with the current version, ACK.

bye,
Sumit

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the patch

@ikerexxe ikerexxe added the backport-to-stable Targets also latest stable branch label Apr 20, 2022
@pbrezina pbrezina added the Ready to push Ready to push label Apr 28, 2022
@pbrezina
Copy link
Member

Pushed PR: #6104

  • master
    • e83e106 - p11_child: enable more than one CRL PEM file
  • sssd-2-7
    • 84e3a8d - p11_child: enable more than one CRL PEM file

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-stable Targets also latest stable branch Bugzilla Pushed
Projects
None yet
5 participants