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

Add TPS Auditor role #376

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Add TPS Auditor role #376

merged 1 commit into from
Jul 28, 2020

Conversation

cipherboy
Copy link
Member

No description provided.

@cipherboy cipherboy added the Bug Bug fixes label Apr 14, 2020
@cipherboy cipherboy self-assigned this Apr 14, 2020
@cipherboy cipherboy marked this pull request as ready for review April 16, 2020 15:40
@cipherboy cipherboy requested review from edewata and jmagne April 16, 2020 15:40
@cipherboy
Copy link
Member Author

OK, firs time doing a LDAP related change. Should I wait for #377 and #378 to merge before continuing? I'm guessing I also need to create upgrade scripts to add this group when missing.

@cipherboy
Copy link
Member Author

Also, does this change need to go in a different ldif file for upgrades?

@edewata edewata requested a review from ladycfu April 16, 2020 15:57
cipherboy
cipherboy commented Apr 16, 2020
View reviewed changes
base/tps/shared/conf/CS.cfg Outdated
@@ -278,7 +278,6 @@ multiroles._001=## multiroles
multiroles._002=##
multiroles.enable=true
multiroles.false.groupEnforceList=Administrators,Auditors,Trusted Managers,Certificate Manager Agents,Registration Manager Agents,Data Recovery Manager Agents,Online Certificate Status Manager Agents,Token Key Service Manager Agents,Enterprise CA Administrators,Enterprise KRA Administrators,Enterprise OCSP Administrators,Enterprise RA Administrators,Enterprise TKS Administrators,Enterprise TPS Administrators,Security Domain Administrators,Subsystem Group,ClonedSubsystems
multiroles.false.groupEnforceList=Administrators,Auditors,Trusted Managers,Certificate Manager Agents,Registration Manager Agents,Data Recovery Manager Agents,Online Certificate Status Manager Agents,Token Key Service Manager Agents,Enterprise CA Administrators,Enterprise KRA Adminstrators,Enterprise OCSP Administrators,Enterprise RA Administrators,Enterprise TKS Administrators,Enterprise TPS Administrators,Security Domain Administrators,Subsystem Group
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, this line lacks a group the previous line has, and also misspells Enterprise KRA Adminstrators (note the missing i after Admin).

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to check which line is actually used by TPS. We can use the Python upgrade framework to fix configuration files, but not database.

edewata
edewata reviewed Apr 16, 2020
View reviewed changes
Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

@cipherboy Basically we need to consider 2 scenarios:

  • new installations
  • upgrading existing instances

If you're adding it to db.ldif, it will take care new installations, but for upgrades you probably need to write something else (e.g. a standalone program). If you do write a standalone program, you might as well use that to handle new installations, but that's up to you.

We don't really have a framework for database upgrade, so you might need to write the code from scratch. Don't use the current Python upgrade framework since it runs during dnf upgrade, and the existing DS may not be running at the time.

For PKI 10.5 I don't think you need to wait for #378 since it's only handling new installations too, but feel free to collaborate with @jmagne for the upgrade case. For PKI 10.9 let's wait for #377 since this probably will become a prototype for a future database upgrade framework.

@ladycfu Other than adding the TPS Auditors LDAP entry, is there anything else that we need to add? I'm not sure how the TPS Auditors will be used.

base/tps/shared/conf/db.ldif Show resolved Hide resolved
base/tps/shared/conf/CS.cfg Outdated Show resolved Hide resolved
@cipherboy cipherboy force-pushed the add-tps-auditor branch 2 times, most recently from 23d09b7 to 96f56a5 Compare April 20, 2020 15:00
@cipherboy cipherboy added the Blocked Blocked due to other dependencies label Apr 20, 2020
@cipherboy
Copy link
Member Author

Blocked on #377 for start of LDAP upgrade framework

@cipherboy
Add TPS auditor
0308eda 
Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy requested a review from edewata July 27, 2020 12:55
@cipherboy cipherboy removed the Blocked Blocked due to other dependencies label Jul 27, 2020
@cipherboy
Copy link
Member Author

Removing blocker label. This is the same PR as #379 for master branch. DB Upgrade framework is still pending and will be for later release, but not including this would be a regression. @edewata Can I merge this?

@cipherboy cipherboy merged commit bf22510 into dogtagpki:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmagne jmagne Awaiting requested review from jmagne

@ladycfu ladycfu Awaiting requested review from ladycfu

@edewata edewata Awaiting requested review from edewata

Assignees

@cipherboy cipherboy

Labels
Bug Bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cipherboy @edewata