#50724 Issue 50701 - Add additional healthchecks to dsconf
Closed by spichugi. Opened by mreynolds.
mreynolds/389-ds-base issue50701  into  master

Download 50724.patch

Description: New checks and several design changes have been implemented

Design changes:
- Moved to a "yield" design, where a lint function can return multiple results
- Revised the lint report so it's easier to read and distiguish between multiple
errors
- Revised most lint errors to include CLI examples on how to fix the issue

New Checks:
- Check TLS certs for expired/expiring
- Add RI plugin checks for missing indexes for RI member attributes
- Added Disk Space check
- Add Virtual Attribute index check
- Add replication agmt status check
- Add replication conflict entry check

relates: https://pagure.io/389-ds-base/issue/50701

It may be better to advise doing a config item reset to default so it upgrades over time rather than settin this manually.

You could consider expanding this text as "remove" vs "swap" may not be clear. So giving an example to say "To remove the conflict and retain the current entry DN" do .... and then have "to restore the conflicted entry, and remove current DN X, do ....".

Is the healtcheck a local or remote tool (dsconf vs dsctl). If it's dsconf, then you can't use _rsa_cert_list because it requires local access.

I think you may add "merge-schemes" as it is possible to define a COS definition without qualifier (it assumes it is 'default').

Is it always true ? If using chaining backend, should not we find a 'chain on update' backend in the mapping tree ?

could it be 'if "eq" in types:' ? (just a question)

Isn't it too alarming ? if RA is still in synchronizing phase it can eventually be in sync ?

Isn't it too alarming ? if RA is still in synchronizing phase it can eventually be in sync ?

The check looks for "in progress" sync status, but I can see the risk for false positives here. Saying this, what is the server code for these messages? maybe they need a time factor IE +- some window of change.

Isn't it too alarming ? if RA is still in synchronizing phase it can eventually be in sync ?

The check looks for "in progress" sync status, but I can see the risk for false positives here. Saying this, what is the server code for these messages? maybe they need a time factor IE +- some window of change.

Yes I have the same concern of false positive. A RA can have transient errors (bind/network/unknown update) or stop (backoff/busy consumer). I healthcheck reports a failure immediately there is a risk that admin will overreact.
I think the search message is good ("in progress") but I would prefer to report (if not "in progress") a WIP status. And report an healthcheck error if WIP status stays for several minutes.

Is it always true ? If using chaining backend, should not we find a 'chain on update' backend in the mapping tree ?

I setup up a database link, and it seems to work fine with the healthcheck. I'm also not finding anything for "chain on update" in the docs.

Also, I did not write this lint function, it was there previously. I just moved it in the source to organize it better

It may be better to advise doing a config item reset to default so it upgrades over time rather than settin this manually.

I don't follow what you asking me to do. The only "default" setting is for the allowed ciphers, not the min and max tls versions

Is the healtcheck a local or remote tool (dsconf vs dsctl). If it's dsconf, then you can't use _rsa_cert_list because it requires local access.

This is all run by dsconf. Hmmm, I'll have to see how to detect this and return a proper message to the user. Might not be so easy if there is an existing DS on the client machine

rebased onto 2c8ac15f8beaa77afef664d39a3b3c52934a9e24

Changes applied please review...

It may be better to advise doing a config item reset to default so it upgrades over time rather than settin this manually.

I don't follow what you asking me to do. The only "default" setting is for the allowed ciphers, not the min and max tls versions

We should recommend deleting the value of mintls/maxtls version so that it does a config-reset and then allows the config on upgrade code to work. I think at the moment you remember setting and changing the version instead?

Is the healtcheck a local or remote tool (dsconf vs dsctl). If it's dsconf, then you can't use _rsa_cert_list because it requires local access.

This is all run by dsconf. Hmmm, I'll have to see how to detect this and return a proper message to the user. Might not be so easy if there is an existing DS on the client machine

You can't detect this. dsconf by design is setup to be remote only. If you want to do "checks" on local filesystem things, it has to be part of dsctl. It's by design in the tool.

Really though, you can do "some" cert checks remotely by parsing the output from the socket, but that's fiddly any annoying.

It may be better to advise doing a config item reset to default so it upgrades over time rather than settin this manually.
I don't follow what you asking me to do. The only "default" setting is for the allowed ciphers, not the min and max tls versions

We should recommend deleting the value of mintls/maxtls version so that it does a config-reset and then allows the config on upgrade code to work. I think at the moment you remember setting and changing the version instead?

But I don't have sslVersionMIn set in my config, NSS defaults to TLS1.0 as the min. So deleting it does not do anything (in my case its not even set). You must set it explicitly until we hard code the minimum in DS to TLS1.2, or NSS set it's minimum to higher value.

Is the healtcheck a local or remote tool (dsconf vs dsctl). If it's dsconf, then you can't use _rsa_cert_list because it requires local access.
This is all run by dsconf. Hmmm, I'll have to see how to detect this and return a proper message to the user. Might not be so easy if there is an existing DS on the client machine

You can't detect this. dsconf by design is setup to be remote only. If you want to do "checks" on local filesystem things, it has to be part of dsctl. It's by design in the tool.
Really though, you can do "some" cert checks remotely by parsing the output from the socket, but that's fiddly any annoying.

Yeah I'll just move the healthcheck from dsconf to dsctl

It may be better to advise doing a config item reset to default so it upgrades over time rather than settin this manually.
I don't follow what you asking me to do. The only "default" setting is for the allowed ciphers, not the min and max tls versions
We should recommend deleting the value of mintls/maxtls version so that it does a config-reset and then allows the config on upgrade code to work. I think at the moment you remember setting and changing the version instead?

But I don't have sslVersionMIn set in my config, NSS defaults to TLS1.0 as the min. So deleting it does not do anything (in my case its not even set). You must set it explicitly until we hard code the minimum in DS to TLS1.2, or NSS set it's minimum to higher value.

I believe William is assuming the default is just OK since "the crypto policies". On EL8 the DEFAULT policy sets TLS1.0; the FUTURE policy sets TLS1.2. Maybe we should recommend setting up the FUTURE (but remind the user it affects the whole system)?

1 new commit added

  • Moved healthcheck to dsctl and added file permission checks

Okay, revised the TLS version lint report, added file permission checks for TLS pin/password files & resolv.conf, and moved healthcheck to dsctl. Please review...

I am missing something with that part of code.
It looks at the csn, but only the maxcsn not all RID. So replication can continue with csn<maxcsn but on different RID. IMHO it could be more difficult looking at RUV than relying on RA status.

  • Incremental update succeeded => In Synchronization
  • Incremental update started => Not in Synchonization: Replication still in progress
  • Error

I am missing something with that part of code.
It looks at the csn, but only the maxcsn not all RID. So replication can continue with csn<maxcsn but on different RID. IMHO it could be more difficult looking at RUV than relying on RA status.

Incremental update succeeded => In Synchronization
Incremental update started => Not in Synchonization: Replication still in progress
Error

Actually healthcheck tool does not have the bind credentials, so it fails to get the consumer ruv, and only relies on the agmt status attributes. See this function is used else where, where you can provide those credentials, but not in this case. Maybe the other use of this function used by "dsconf INSTNACE repl-agmt-status --bind-dn=##### --bind-pw=#### AGMT_DN --suffix SUFFIX" is not accurate then, but for healthcheck it only uses the status attribute.

So the healthcheck tool solely relies on the agtm status attribute, and the JSON attribute already determines the severity level for you. See agmt_set_last_update_status() in repl5_agmt.c and how it it prepares the JSON message (see "state").

Looks good to me, that is at least for the TLS part. Thanks.

@mreynolds, thanks for the explanations. This is also looking good to me.

rebased onto a370f8d45d17a3c06931e57a447b9765b97c17e2

Pull-Request has been merged by mreynolds

It may be better to advise doing a config item reset to default so it upgrades over time rather than settin this manually.
I don't follow what you asking me to do. The only "default" setting is for the allowed ciphers, not the min and max tls versions
We should recommend deleting the value of mintls/maxtls version so that it does a config-reset and then allows the config on upgrade code to work. I think at the moment you remember setting and changing the version instead?
But I don't have sslVersionMIn set in my config, NSS defaults to TLS1.0 as the min. So deleting it does not do anything (in my case its not even set). You must set it explicitly until we hard code the minimum in DS to TLS1.2, or NSS set it's minimum to higher value.

I believe William is assuming the default is just OK since "the crypto policies". On EL8 the DEFAULT policy sets TLS1.0; the FUTURE policy sets TLS1.2. Maybe we should recommend setting up the FUTURE (but remind the user it affects the whole system)?

No, that's not my point.

The remedial action says to set a tlsmin value when we should say they should remove the value (ie do a default-value reset) instead. That way we can use the system policies or confif-on-uphate.

If we recommend people set it, they'll just set the value and it will need to forever be "set" periodically by an admin.

Our remedial actions should be "do once, fix forever".

"unable to query the backend" should be the correct text here :)

Anyway, I think moving healtcheck to dsctl makes a lot of sense :)

It may be better to advise doing a config item reset to default so it upgrades over time rather than settin this manually.
I don't follow what you asking me to do. The only "default" setting is for the allowed ciphers, not the min and max tls versions
We should recommend deleting the value of mintls/maxtls version so that it does a config-reset and then allows the config on upgrade code to work. I think at the moment you remember setting and changing the version instead?
But I don't have sslVersionMIn set in my config, NSS defaults to TLS1.0 as the min. So deleting it does not do anything (in my case its not even set). You must set it explicitly until we hard code the minimum in DS to TLS1.2, or NSS set it's minimum to higher value.
I believe William is assuming the default is just OK since "the crypto policies". On EL8 the DEFAULT policy sets TLS1.0; the FUTURE policy sets TLS1.2. Maybe we should recommend setting up the FUTURE (but remind the user it affects the whole system)?

No, that's not my point.
The remedial action says to set a tlsmin value when we should say they should remove the value (ie do a default-value reset) instead. That way we can use the system policies or confif-on-uphate.
If we recommend people set it, they'll just set the value and it will need to forever be "set" periodically by an admin.
Our remedial actions should be "do once, fix forever".

Sure, I understood, and I agree we should require as little values to be set as possible. However, our sense of security is stronger than the one of the latest stable used crypto libraries'. If we would want to have the "do once, fix forever" and at the same time have stronger default than the distro's default, then we would have to keep hardcoding (and updating over time) our default -- is this what we want? (IDK what's the default in distros other than Fedora/CentOS, so bear with me please.)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3779

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

Metadata