add CRL-based client certificate revocation check

[Jan-Schuppik]

Add a crlChecker that loads a PEM or DER Certificate Revocation List, verifies its signature against the CA, and watches the file for live updates via fsnotify. Atomic file replacements (mv) are handled by re-adding the watch path on Create/Rename events.

Expose ApplyRevocationCheck on TLSCommon, which wires the checker into tls.Config.VerifyConnection so revoked client certificates are rejected at handshake time.

resolves #218

refs Icinga/icinga-notifications#454

[yhabteab]

I've nothing else to complain about here apart from the below inline comment but since this was requested by Alvar and Johannes, I'll let them review and approve it.

/cc @oxzi

[yhabteab]

Since I didn't understand what this comment is all about at first, I've exprimented a bit in my own branch but still don't get what the fsnotify.Create event is needed for. If the configured file doesn't exist then you shouldn't even be able to add it to the watch list and would already crash on the first watcher.Add() call on line 78, so I don't know what this event is good for. Please see the changes I made in the other branch and verify if that should cover all use cases we need.

[oxzi]

I guess the Create event might be relevant in the following scenario:

1. Status quo: A CRL lies on the disk.
2. The sysadmin scps a new CRL to the machine, stored somewhere in its home directory or in the temp.
3. The sysadmin removes the old CRL - the Remove event triggers
4. The sysadmin moves the CRL to the location - the Create should trigger again

However, this requires the changes from https://github.com/Icinga/icinga-go-library/pull/219/changes#r3569471676 to work.

[oxzi]

Why is this in TLSCommon and not in ServerTLS?

Furthermore, there should be a check in the Validate method to ensure that iff CrlFile is set, Ca must be set as well. Also, it must be ensured that either tls.VerifyClientCertIfGiven or tls.RequireAndVerifyClientCert are the tls.ClientAuthType, as otherwise this code would be a noop - but without any visible failure to the client.

[Jan-Schuppik]

It's placed in TLSCommon rather than ServerTLS since CRL checking isn't inherently server-side, clients could use it as well. It hasn't been configured for client TLS yet, but keeping it common avoids duplicating the field/config plumbing if it's needed there.

[oxzi]

Suggested change

	return nil // no client cert presented — skip
	return nil // no client cert presented - skip

[oxzi]

This read lock only ensures that there are no races between acquiring the crl and setting it in the reload method. However, the caller might proceed with an outdated CRL.

As this method is only used in IsRevoked, why not remove it, and create a read lock in IsRevoked while iterating over the entries?

[oxzi]

According to Decode's API doc, block might be nil. Passing nil to x509.ParseCertificate might panic. Please add a nil check against invalid CA certificates. Btw, this check is present in CrlChecker.reload.

[oxzi]

If the CRL's NextUpdate time has passed, the file is reloaded before the lookup so the check always operates on fresh data.

This does not happen. Your code returns an error.

[oxzi]

Suggested change

	if err != nil {
	return fmt.Errorf("cannot create watcher: %w", err)
	}
	if err := watcher.Add(c.path); err != nil {
	_ = watcher.Close()
	return fmt.Errorf("cannot watch CRL file: %w", err)
	}
	defer func() { _ = watcher.Close() }()
	if err != nil {
	return fmt.Errorf("cannot create watcher: %w", err)
	}
	defer func() { _ = watcher.Close() }()
	if err := watcher.Add(c.path); err != nil {
	return fmt.Errorf("cannot watch CRL file: %w", err)
	}

[oxzi]

At least for a fsnotify.Remove event, the reload method is likely to fail - unless there is a race due to multiple similar events.

[oxzi]

I guess the Create event might be relevant in the following scenario:

1. Status quo: A CRL lies on the disk.
2. The sysadmin scps a new CRL to the machine, stored somewhere in its home directory or in the temp.
3. The sysadmin removes the old CRL - the Remove event triggers
4. The sysadmin moves the CRL to the location - the Create should trigger again

However, this requires the changes from https://github.com/Icinga/icinga-go-library/pull/219/changes#r3569471676 to work.

[oxzi]

For a fsnotify.Remove event, this will error out. Afterwards there is nothing to be watched left!

Take the following demo as an example.

package main

import (
        "fmt"

        "github.com/fsnotify/fsnotify"
)

func errCheck(err error) {
        if err != nil {
                panic(err)
        }
}

func main() {
        const f = "test"

        watcher, err := fsnotify.NewWatcher()
        errCheck(err)
        defer func() { _ = watcher.Close() }()

        errCheck(watcher.Add(f))
        fmt.Printf("Starting wiht: %v\n", watcher.WatchList())

        for event := range watcher.Events {
                fmt.Printf("%+v\nAdd: %v\nAll :%v\n\n", event, watcher.Add(f), watcher.WatchList())
        }
}

$ go build
$ ./foo &
[1] 516727
Starting wiht: [test]
$ echo a > test
WRITE         "test"
Add: <nil>
All :[test]

mv test{,bkp}
RENAME        "test"
Add: no such file or directory
All :[]

$ touch test
$ # nope, nothing. we are watching nothing. the CRL is lost now

---

To fix this, watch the CRL's parent directory, but only act upon the CRL file. Take this updated demo as an example.

package main

import (
        "fmt"
        "path/filepath"

        "github.com/fsnotify/fsnotify"
)

func errCheck(err error) {
        if err != nil {
                panic(err)
        }
}

func main() {
        const f = "./test"

        watcher, err := fsnotify.NewWatcher()
        errCheck(err)
        defer func() { _ = watcher.Close() }()

        errCheck(watcher.Add(filepath.Dir(f)))
        fmt.Printf("Starting wiht: %v\n", watcher.WatchList())

        for event := range watcher.Events {
                if filepath.Base(event.Name) != filepath.Base(f) {
                        fmt.Printf("[SKIP] %+v\n", event)
                        continue
                }
                fmt.Printf("[ADD ] %+v\nAdd: %v\nAll :%v\n\n", event, watcher.Add(f), watcher.WatchList())
        }
}

$ go build
$ ./foo &
[2] 545169
Starting wiht: [.]

$ echo a > test
[ADD ] WRITE         "./test"
Add: <nil>
All :[. test]

$ mv test{,bkp}
[ADD ] RENAME        "./test"
Add: no such file or directory
All :[. test]

[SKIP] CREATE        "./testbkp" ← "./test"
[ADD ] RENAME        "test"
Add: no such file or directory
All :[.]

$ touch test
[ADD ] CREATE        "./test"
Add: <nil>
All :[. test]

[ADD ] CHMOD         "./test"
Add: <nil>
All :[. test]