Implement simple CA range checking - #77
Conversation
|
@frasertweedale can you take a look? |
8ed261d to
6e849a6
Compare
frasertweedale
left a comment
frasertweedale
left a comment
There was a problem hiding this comment.
@rcritten the general approach seems fine, as is most of the code. But there are some mistakes and omissions (I do not blame you for this, it is confusing!) If you agree, I can take over the ticket for the next sprint and make the necessary fixes and additions? It is good for me to get more familiar with Health Check.
| yield Result(self, constants.CRITICAL, | ||
| key='nextRange', | ||
| dn='ou=certificateRepository,ou=ca,o=ipaca', | ||
| msg='multiple {key} values in {dn}') |
There was a problem hiding this comment.
This seems to also handle the len(entries) == 0 case, so perhaps the error message should mention the number of entries (and that only one entry was expected).
There was a problem hiding this comment.
The no entries case is handled on line 67. The message for multiple can be improved, I'll push an update.
| msg='multiple {key} values in {dn}') | ||
| return | ||
| else: | ||
| nextrange = int(entries[0].single_value.get('nextRange', 0)) |
There was a problem hiding this comment.
Certificate repository (and KRA keyRepository) uses hex encoding. Even though they may look decimal-ish :)
| attrlist = ['beginRange', 'endRange', 'host'] | ||
| try: | ||
| entries = self.conn.get_entries( | ||
| DN('ou=requests,ou=ranges,o=ipaca'), |
There was a problem hiding this comment.
So here we are comparing request ranges against the certificateRepository nextRange. We need to output and compare range entries and nextRange values separate for each of:
- CA certificateRepository
- CA requests
- CA replicas
- KRA keyRepository
- KRA requests
- KRA replicas
It's a pain, I know. I suppose the best way is a superclass with all the logic, and a subclass for each managed range that just plugs in a few parameters (what are the CS.cfg params, repository object DN, range object base DN, whether the numbers are decimal or hex).
There was a problem hiding this comment.
Ah ok, I didn't get that part from the docs.
| keys = [('dbs.beginSerialNumber', True), | ||
| ('dbs.endSerialNumber', True), | ||
| ('dbs.nextBeginSerialNumber', False), | ||
| ('dbs.nextEndSerialNumber', False)] |
There was a problem hiding this comment.
And yeah, same comment here, this looks fine for serials, but we need to do similar thing for all the other managed ranges.
|
Sure, you can take it over. I'll push the one minor update for the multiple keys message. |
Checking the CS.cfg ranges requires visibility for the whole cluster so only report those values. We can check that the data stored in LDAP doesn't overlap and that the nextRange value is defined. freeipa#76
|
I pushed a fix for multiple ranges and added a test case for it. |
2 participants
Checking the CS.cfg ranges requires visibility for the whole
cluster so only report those values.
We can check that the data stored in LDAP doesn't overlap and
that the nextRange value is defined.
#76