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.

Sign up for GitHub

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

Jump to bottom

Distinguished names comparator [18789] #3559

Merged
merged 10 commits into from
Jun 22, 2023
Merged

Distinguished names comparator [18789] #3559

merged 10 commits into from
Jun 22, 2023

Conversation

jparisu
Copy link
Contributor

@jparisu jparisu commented Jun 1, 2023 •
edited by Mario-DL
Loading

Reimplement Distinguished names comparator function rfc2253_string_compare.

Description

There was an incorrect behavior in the comparison of Distinguished Names in security accesscontrol module.
Main problem is that RFC 2253 was not correctly followed, and some Distinguished Names that should be accepted were not (mainly because disorder of value-types attributes was not contemplated).

This PR have the following features in respective commits:

  1. Extend a function that made security fail without log information (35cd6225ae72188100bc04edf9cc06e674053776)
  2. Implement new rfc2253_string_compare function (c80227739b7842dbc7428da9e73b338881989cb7)
  3. Add tests for such function (270c155eca0fdbb2c003a1ea5d8be459d79cfccf)

NOTE: The implementation of this function is highly non trivial as many concerns must be taken into account, as all RFC 2253 details and zero-allocation policy.

NOTE: This implementation is not complete, but at least extender than before. A future refactor must be done to handle Distinguished Names appropriately.

Documentation

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • [N/A] New feature has been added to the versions.md file (if applicable).
  • [N/A] New feature has been documented/Current behavior is correctly described in the documentation.
  • Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@jparisu jparisu force-pushed the fix/secutity-distinguished-names branch from 270c155 to f341973 Compare June 1, 2023 10:42
@JLBuenoLopez JLBuenoLopez added this to the v2.11.0 milestone Jun 5, 2023
This was referenced Jun 5, 2023
Merged
Merged
@MiguelCompany
Copy link
Member

@richiprosima Please test this

@jparisu jparisu changed the title Distinguished names comparator Distinguished names comparator [18789] Jun 13, 2023
@jparisu
Copy link
Contributor Author

jparisu commented Jun 13, 2023

@richiprosima Please test this

@jparisu jparisu closed this Jun 13, 2023
@jparisu jparisu reopened this Jun 13, 2023
@jparisu
Copy link
Contributor Author

jparisu commented Jun 13, 2023

@richiprosima Please test this

@JesusPoderoso
Copy link
Contributor

JesusPoderoso commented Jun 15, 2023 •
edited
Loading

@jparisu Please take a look at the uncrustify issue.

Is the Windows failed test related with the PR?

Update: documentation CI did not pass due to version conflicts. Rebasing master would solve that

@JesusPoderoso JesusPoderoso added the in progress Issue or PR which is being reviewed label Jun 15, 2023
@jparisu jparisu force-pushed the fix/secutity-distinguished-names branch from c5fb837 to 00b2761 Compare June 20, 2023 06:23
@jparisu
Copy link
Contributor Author

jparisu commented Jun 20, 2023

@JesusPoderoso

Please take a look at the uncrustify issue.

00b2761

Is the Windows failed test related with the PR?

It does not seem so

Update: documentation CI did not pass due to version conflicts. Rebasing master would solve that

Let's see if it works now

@jparisu
Copy link
Contributor Author

jparisu commented Jun 20, 2023

@richiprosima Please test this

@Mario-DL Mario-DL added ci-pending PR which CI is running and removed in progress Issue or PR which is being reviewed labels Jun 20, 2023
@Mario-DL
Copy link
Member

@richiprosima Please test this

Mario-DL
Mario-DL requested changes Jun 20, 2023
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR serves as a nice initial implementation of the RFC 2253 Distinguished names.
The implementation is not complete yet and could be enhanced by using classes instead of direct strings but it really brings more robustness and improves the old way of performing string comparisons within the security accescontrol. Good job !

Just leaving some suggestions and NITs

src/cpp/rtps/security/SecurityManager.cpp Outdated Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.h Outdated Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.h Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.cpp Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.h Outdated Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.cpp Outdated Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.cpp Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.h Outdated Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.h Outdated Show resolved Hide resolved
src/cpp/security/accesscontrol/DistinguishedName.h Show resolved Hide resolved
apply suggestions
96b94dc 
Signed-off-by: jparisu <[email protected]>
Mario-DL
Mario-DL requested changes Jun 20, 2023
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

A NIT. Please upload also the fix for the linter

apply small suggestions
9c169dd 
Signed-off-by: jparisu <[email protected]>
jparisu added 2 commits June 20, 2023 16:39
@jparisu jparisu force-pushed the fix/secutity-distinguished-names branch from 04ddb79 to 095566e Compare June 21, 2023 06:37
undo improving log info
a7b21a7 
Signed-off-by: jparisu <[email protected]>
@rsanchez15
Copy link
Contributor

@richiprosima Please test this

Mario-DL
Mario-DL approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

LGTM with Green CI

@Mario-DL
Copy link
Member

@richiprosima please test this

@Mario-DL
Copy link
Member

Failed test not related with this PR. mac compilation and tests were checked before in 00b2761.

@Mario-DL Mario-DL added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Jun 22, 2023
@EduPonz EduPonz merged commit 1114723 into master Jun 22, 2023
@EduPonz EduPonz deleted the fix/secutity-distinguished-names branch June 22, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mario-DL Mario-DL Mario-DL approved these changes

@MiguelCompany MiguelCompany Awaiting requested review from MiguelCompany

Assignees

@MiguelCompany MiguelCompany

Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.11.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jparisu @MiguelCompany @JesusPoderoso @Mario-DL @rsanchez15 @EduPonz @JLBuenoLopez