Map Server as a Service

[juagargi]

There should exist a mapserver binary that can be run as a service, with its systemd service file and all.

Tasks TODO:

• Merge New DB design #43
• Create new binary that represents the service
• Create configuration under /etc/fpki/mapserver.conf
• Allow shutting down via SIGTERM
• Automatically find the current size and retrieve remaining
• Create timer for pruning
• Create timer for updating
• Examine the options to report errors

This PR closes #26

---

This change is 

[juagargi]

[x] Examine the options to report errors

We can use Prometheus (ideal) or a script to send an email in our server if the systemd service crashes.
For now, I've created a systemd service file.

[juagargi]

Reviewable status: 165 of 193 files reviewed, 8 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/db/mysql/conf.go line 68 at r3 (raw file):

Previously, cyrill-k wrote…

what is this field used for?

To allow the driver to parse its internal representation into Go's time.Time

---

pkg/db/mysql/mysql_test.go line 306 at r3 (raw file):

Previously, cyrill-k wrote…

"... two leaves ..."

Done.

---

pkg/mapserver/updater/updater.go line 99 at r3 (raw file):

Previously, cyrill-k wrote…

Should we also check that the returned certificates are really the ones added by the CT log server (i.e., verify MHT inclusion proof)?
Or, if we postpone doing this check for later, should we already add a placeholder function that does this and maybe the necessary data structures? Or just add a todo?
It would require to do one more request to the CT log (https://datatracker.ietf.org/doc/html/rfc6962#page-19) to get the consistency proof between two signed tree heads (last index written and current index) and to store the signed tree head at the last updated index.

Done.
As discussed offline, this is only partially implemented by just leaving the real verification to be done later, and flagged with two TODO.

---

pkg/util/time.go line 32 at r3 (raw file):

Previously, cyrill-k wrote…

incomplete sentence

Done. Removed complete function, as it was a leftover of a previous attempt.

---

tests/integration/domainowner_pca_policlog_interaction/main.go line 3 at r3 (raw file):

Previously, cyrill-k wrote…

should this file be completely removed?

I want to keep it in master, as we should eventually bring a proper integration test for the domain owner and the PCA interactions. I assume that this could help, but let me know if you prefer to have it removed.

---

cmd/mapserver/config.go line 12 at r3 (raw file):

Previously, cyrill-k wrote…

Should we already support fetching from multiple CT logs?
Also, do we need the id and public key of the log to verify the MHT inclusion proof in the CT log's append-only data structure?

Done.

---

cmd/mapserver/mapserver.go line 112 at r3 (raw file):

Previously, cyrill-k wrote…

This is not really a complaint, it is most likely just my ignorance with golang parallelism ;D
But why did you not just normally return the error for the prune and update function below and only use the updateErrChan here and in PruneAndUpdate?
Wouldn't that make the code easier to understand?

You are right. The function pruneAndUpdate is unnecessarily complex; it's now simpler by having prune and update directly return the error. Additionally, the previous code there had a bug.

As for the updateChan, we need it because the function could be called from several goroutines at the same time, and we want them to wait. Another sync mechanism like a mutex would have worked as well.

---

cmd/mapserver/mapserver.go line 137 at r3 (raw file):

Previously, cyrill-k wrote…

Don't we have to abort if an error was returned? (i.e., add a return statement here)?

Done.

[cyrill-k]

Reviewed 32 of 32 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 176 at r4 (raw file):

	parseFunction := func(fields []string, lineNo int) error {
		// First avoid even parsing already expired certs.

One thing to keep in mind is that in some cases, the csv fields are not properly escaped, which may lead to issues because the expiration column is shifted. We could implement a sanity check by counting the number of columns and not use the expiration column if the count is larger than 8?

---

pkg/mapserver/logfetcher/logfetcher.go line 116 at r4 (raw file):

	return State{
		Size: sth.TreeSize,
		STH:  sth.TreeHeadSignature.Signature,

Shouldn't this be the complete STH and not just the signature?
Because I think the SHA256RootHash may also be needed during verification.
Also, the signature is either (1) already verified by the GetSTH() function or (2) must be verified later in which case all the other STH fields must also be provided to perform the verification.

---

pkg/mapserver/updater/updater.go line 106 at r4 (raw file):

	// intermediate step).
	// We may want to start a transaction in the DB at the StartFetchingRemaining and commit it
	// only if the verification of all batches is alright.

That's a good point.
One potential workaround for this is to perform the append-only verification in two steps: one for the batch entries and one for the entries after the batch before the current size.
Let N = old tree size, B = batch size and M (> N + B) = the current size. We request two consistency proofs from the CT log: [N, N+B] and [N+B, M]. First, we verify that appending the certificates in the batch results in the correct MHT root value at N+B. Then, we verify that the append-only property between N+B and M is satisfied (without looking at the actual certificates that it contains)

The only potential issue that I see is that a CT log server may not provide consistency proofs for arbitrary ranges within [N, M], since the RFC states that "Both tree sizes must be from existing v1 STHs (Signed Tree Heads)." (see https://datatracker.ietf.org/doc/html/rfc6962#section-4.4). Although, when I checked with the argon 2023 log, it seemed to work for arbitrary ranges (https://ct.googleapis.com/logs/argon2023/ct/v1/get-sth-consistency?first=1000000&second=2000011)

---

pkg/mapserver/updater/updater.go line 217 at r4 (raw file):

	ctx context.Context,
	leafCerts []*ctx509.Certificate,
	chains [][]*ctx509.Certificate,

It may be more efficient to pass the actual leaf content to this function instead of recreating the CT log leaf data structure. But this would probably need to be evaluated once this function is implemented, so I would suggest to just leave a comment.

---

tests/integration/domainowner_pca_policlog_interaction/main.go line 3 at r3 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

I want to keep it in master, as we should eventually bring a proper integration test for the domain owner and the PCA interactions. I assume that this could help, but let me know if you prefer to have it removed.

No, I don't have any strong preferences. Either way is fine for me.

---

cmd/mapserver/config.go line 12 at r3 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done.

How about the id and public key of the log to verify the MHT inclusion proof? Shouldn't this also be added in the config file?

[juagargi]

Reviewable status: all files reviewed, 6 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 176 at r4 (raw file):

Previously, cyrill-k wrote…

One thing to keep in mind is that in some cases, the csv fields are not properly escaped, which may lead to issues because the expiration column is shifted. We could implement a sanity check by counting the number of columns and not use the expiration column if the count is larger than 8?

Do you have any records as an example? Maybe we can easily heal them.

---

pkg/mapserver/logfetcher/logfetcher.go line 116 at r4 (raw file):

Previously, cyrill-k wrote…

Shouldn't this be the complete STH and not just the signature?
Because I think the SHA256RootHash may also be needed during verification.
Also, the signature is either (1) already verified by the GetSTH() function or (2) must be verified later in which case all the other STH fields must also be provided to perform the verification.

I have no idea what will be needed honestly. I've added the STH mainly as a placeholder. But same as with the configuration, the implementer may need to serialize the complete TreeHeadSignature to bytes and back. I think for now it is clear that the info can be obtained and transmitted to the appropriate location in the code.

---

pkg/mapserver/updater/updater.go line 106 at r4 (raw file):

Previously, cyrill-k wrote…

That's a good point.
One potential workaround for this is to perform the append-only verification in two steps: one for the batch entries and one for the entries after the batch before the current size.
Let N = old tree size, B = batch size and M (> N + B) = the current size. We request two consistency proofs from the CT log: [N, N+B] and [N+B, M]. First, we verify that appending the certificates in the batch results in the correct MHT root value at N+B. Then, we verify that the append-only property between N+B and M is satisfied (without looking at the actual certificates that it contains)

The only potential issue that I see is that a CT log server may not provide consistency proofs for arbitrary ranges within [N, M], since the RFC states that "Both tree sizes must be from existing v1 STHs (Signed Tree Heads)." (see https://datatracker.ietf.org/doc/html/rfc6962#section-4.4). Although, when I checked with the argon 2023 log, it seemed to work for arbitrary ranges (https://ct.googleapis.com/logs/argon2023/ct/v1/get-sth-consistency?first=1000000&second=2000011)

My thought was: we cannot be confident that the two STHs (old and new one) are correct until we request the new one. And having the new STH way in the future size is not helping verify each batch. We can get consistency proofs for the batches, but it will yield intermediate top nodes that cannot be verified, because we won't get the whole TreeSignedHead structure in the intermediate proofs (or will we? with the public signature and all? The student taking the project should test this, and read the RFC)

---

pkg/mapserver/updater/updater.go line 217 at r4 (raw file):

Previously, cyrill-k wrote…

It may be more efficient to pass the actual leaf content to this function instead of recreating the CT log leaf data structure. But this would probably need to be evaluated once this function is implemented, so I would suggest to just leave a comment.

True, added comment

---

tests/integration/domainowner_pca_policlog_interaction/main.go line 3 at r3 (raw file):

Previously, cyrill-k wrote…

No, I don't have any strong preferences. Either way is fine for me.

I'm removing it 😃

---

cmd/mapserver/config.go line 12 at r3 (raw file):

Previously, cyrill-k wrote…

How about the id and public key of the log to verify the MHT inclusion proof? Shouldn't this also be added in the config file?

I would let the implementer choose how to do it, but yes, this should be part of the config as well. Since we are not needing it at the moment, I prefer not to modify it until we know what we want exactly.

[cyrill-k]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 176 at r4 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Do you have any records as an example? Maybe we can easily heal them.

I couldn't find an example in the Alexa top 100K certificates, but the problem was that the last two columns (NotBefore and NotAfter) are shifted if the common name (or a SAN entry) contains one of multiple commas. As a workaround, we could simply use the last entry for the expiration, i.e., fields[len(fields)-(8-ExpirationColumn)]

---

pkg/mapserver/logfetcher/logfetcher.go line 116 at r4 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

I have no idea what will be needed honestly. I've added the STH mainly as a placeholder. But same as with the configuration, the implementer may need to serialize the complete TreeHeadSignature to bytes and back. I think for now it is clear that the info can be obtained and transmitted to the appropriate location in the code.

I see, in that case I would suggest to add a comment that we may need to change this field to implement the CT log verification.

---

pkg/mapserver/updater/updater.go line 106 at r4 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

My thought was: we cannot be confident that the two STHs (old and new one) are correct until we request the new one. And having the new STH way in the future size is not helping verify each batch. We can get consistency proofs for the batches, but it will yield intermediate top nodes that cannot be verified, because we won't get the whole TreeSignedHead structure in the intermediate proofs (or will we? with the public signature and all? The student taking the project should test this, and read the RFC)

Maybe I can reformulate it:
Again, let N = old tree size, B = batch size and M (> N + B) = the current size.

Ideally, we would like to do the verification per batch, which works as follows:
(1) Request STH at N+B with MHT root value R
(2) Add the certificates in the batch [N, N+B] and calculate the new MHT root value R'
(2) Verify that adding these certificates results results in the correct MHT root value (R==R')
(3) Accept batch certificates and add to the DB

However, since we cannot fetch the STH at arbitrary tree sizes, your current proposal is to accumulate all batches between N and M and then verify that the root value is correct:
(1) Request STH at M with MHT root value R
(2) Start DB transaction
(3) for all batches ([N,N+B], [N+B,N+2B],...[N+XB,M]): add certificates in batch and calculate the final root value R' at tree size M
(4) Verify that adding all batches results in the correct MHT root value (R==R')
(5) Commit DB transaction

However, I'm not sure how well this will work because we have to lock tables, have huge transactions, etc. My suggestion is the following:
(1) Request STH at M with MHT root value R
(2) for all batches except the last one ([N, N+B], [N+B, N+2B], ..., [N+(X-1)B, N+XB]), do the following:
(3a) for the batch [S, S+B], add all certificates in the batch which results in a new MHT root value R'
(3b) request consistency proof for [S+B, M] (as I said before, it is not clear from the RFC if this must be supported by a CT log servers)
(3c) verify that the consistency proof is valid given the two tree roots R' at S+B and R at M
(3d) if the consistency proof is valid, add the batch to the DB
(4) Verify and add the last batch [N+XB, M] as described in the first approach

The idea is that due to the append-only property of the MHT, it is not a problem if the MHT tree root at R' in (3c) is not signed by an STH.
Because if the attacker can fabricate a batch B_0 (resulting in the root value R_0') which is different from the actual batch B_1 (resulting in to root value R_1'), such that there exist consistency proofs for both root values (consistency [R_0', R] and consistency [R_1', R]), the append-only property of the MHT would be violated.
Someone should properly verify this.

This became much longer than I expected 😅
I would suggest that we merge this as it is right now and create a new gitlab issue for all concerns regarding the (efficient) validation of CT log entries and copy all the discussion there.
What do you think?

---

cmd/mapserver/config.go line 12 at r3 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

I would let the implementer choose how to do it, but yes, this should be part of the config as well. Since we are not needing it at the moment, I prefer not to modify it until we know what we want exactly.

Ok 👍

[juagargi]

Reviewable status: all files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 176 at r4 (raw file):

Previously, cyrill-k wrote…

I couldn't find an example in the Alexa top 100K certificates, but the problem was that the last two columns (NotBefore and NotAfter) are shifted if the common name (or a SAN entry) contains one of multiple commas. As a workaround, we could simply use the last entry for the expiration, i.e., fields[len(fields)-(8-ExpirationColumn)]

Done.
But we need to find examples like those you mention, just to check that nothing else will be broken.

---

pkg/mapserver/logfetcher/logfetcher.go line 116 at r4 (raw file):

Previously, cyrill-k wrote…

I see, in that case I would suggest to add a comment that we may need to change this field to implement the CT log verification.

Done.

---

pkg/mapserver/updater/updater.go line 106 at r4 (raw file):

Previously, cyrill-k wrote…

Maybe I can reformulate it:
Again, let N = old tree size, B = batch size and M (> N + B) = the current size.

Ideally, we would like to do the verification per batch, which works as follows:
(1) Request STH at N+B with MHT root value R
(2) Add the certificates in the batch [N, N+B] and calculate the new MHT root value R'
(2) Verify that adding these certificates results results in the correct MHT root value (R==R')
(3) Accept batch certificates and add to the DB

However, since we cannot fetch the STH at arbitrary tree sizes, your current proposal is to accumulate all batches between N and M and then verify that the root value is correct:
(1) Request STH at M with MHT root value R
(2) Start DB transaction
(3) for all batches ([N,N+B], [N+B,N+2B],...[N+XB,M]): add certificates in batch and calculate the final root value R' at tree size M
(4) Verify that adding all batches results in the correct MHT root value (R==R')
(5) Commit DB transaction

However, I'm not sure how well this will work because we have to lock tables, have huge transactions, etc. My suggestion is the following:
(1) Request STH at M with MHT root value R
(2) for all batches except the last one ([N, N+B], [N+B, N+2B], ..., [N+(X-1)B, N+XB]), do the following:
(3a) for the batch [S, S+B], add all certificates in the batch which results in a new MHT root value R'
(3b) request consistency proof for [S+B, M] (as I said before, it is not clear from the RFC if this must be supported by a CT log servers)
(3c) verify that the consistency proof is valid given the two tree roots R' at S+B and R at M
(3d) if the consistency proof is valid, add the batch to the DB
(4) Verify and add the last batch [N+XB, M] as described in the first approach

The idea is that due to the append-only property of the MHT, it is not a problem if the MHT tree root at R' in (3c) is not signed by an STH.
Because if the attacker can fabricate a batch B_0 (resulting in the root value R_0') which is different from the actual batch B_1 (resulting in to root value R_1'), such that there exist consistency proofs for both root values (consistency [R_0', R] and consistency [R_1', R]), the append-only property of the MHT would be violated.
Someone should properly verify this.

This became much longer than I expected 😅
I would suggest that we merge this as it is right now and create a new gitlab issue for all concerns regarding the (efficient) validation of CT log entries and copy all the discussion there.
What do you think?

Done.
Issue is #47, referenced also on a comment in the code.

---

cmd/mapserver/config.go line 12 at r3 (raw file):

Previously, cyrill-k wrote…

Ok 👍

Done.

[cyrill-k]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 302 at r6 (raw file):

	s := strings.Split(fields[expirationColumn], ".")
	if len(s) != 2 {
		return 0, fmt.Errorf("unrecognized timestamp in 7th column: %s", fields[expirationColumn])

this should probably be "... in the last column: ..."

[juagargi]

Reviewable status: 187 of 195 files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 302 at r6 (raw file):

Previously, cyrill-k wrote…

this should probably be "... in the last column: ..."

Done.

[cyrill-k]

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 302 at r6 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done.

I don't believe you ;D

---

pkg/mapserver/mapserver.go line 248 at r7 (raw file):

// of all requested IDs.
// Since each ID is 32 bytes, the hex string will always be a multiple of 64.
func (s *MapServer) apiGetPolicyPayloads(w http.ResponseWriter, r *http.Request) {

This is one of the issues that I also fixed in my local version ;)
However, instead of splitting it into two functions (one for certs and one for policies), I made one API call that returns both certificate and policy payload objects based on their respective hashes.
I think we can merge this for now and then I'll also add my version in a later PR.

[juagargi]

Reviewable status: 194 of 195 files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

cmd/ingest/processor.go line 302 at r6 (raw file):

Previously, cyrill-k wrote…

I don't believe you ;D

Done. And committed. And pushed. 😄 now for real.

---

pkg/mapserver/mapserver.go line 248 at r7 (raw file):

Previously, cyrill-k wrote…

This is one of the issues that I also fixed in my local version ;)
However, instead of splitting it into two functions (one for certs and one for policies), I made one API call that returns both certificate and policy payload objects based on their respective hashes.
I think we can merge this for now and then I'll also add my version in a later PR.

SGTM

[cyrill-k]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @juagargi)