New DB design

[cyrill-k]

This pull requests modifies the structure of the DB.
The goals of the new design are:

• Updates become faster by not needing to retrieve data from the DB, just push.
• We allow to expire certificates: the expiration time is visible in the certs table layout.
• The Sparse Merkle Tree is built using the hashes of the certificates and policies, not their payloads. This allows for a much faster update after a new certificate/policy has been added or modified. The hashes of the payloads and their aggregation into the domain field is done in the DB engine side, so there is no need to transmit data.
  Closes: Root node should be stored in DB #27, Ingest Tool #28, DB Design prevents Concurrency #29, Downloader needs to obtain the trust chain #39, Downloader needs to throttle whenever the server indicates so #40

This change is 

[cyrill-k]

Reviewed 3 of 148 files at r1.
Reviewable status: 3 of 148 files reviewed, 6 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/structure.go line 10 at r1 (raw file):

// PolicyObject is an interface that is implemented by all objects that are part of the set
// of "policy objects". A policy object is that one that represents functionality of policies
// for a domain, such as RPC, RCSR, SPT, SPRT, SP, PSR or Policy.

I'm not too happy with the currently used policy object names, as I think they are quite confusing.
I would suggest to rename them in a more consistent manner.
Should we do this in this PR or a separate one?

---

pkg/common/structure.go line 11 at r1 (raw file):

// of "policy objects". A policy object is that one that represents functionality of policies
// for a domain, such as RPC, RCSR, SPT, SPRT, SP, PSR or Policy.
type PolicyObject interface {

I'm wondering whether it is the correct abstraction to group all of these objects together.
My suggestion is to group them as follows since they'll have more common fields that way:

1. Policy objects that can be identified (and thus returned) by the mapserver and thus the DB: RPC, SP, PCRevocation
2. Embedded policy objects: SPT, SPRT
3. Objects related to issuing policy objects, that are not relevant for the mapserver (but for the PCA): RCSR, PSR, Revocation Signing Request (doesn't exist yet and is maybe not needed)

The first two object types could be combined as signed policy objects and have the following common fields: Version, Issuer, Signature, SignatureAlgorithm
The first object type could then additionally have the common fields: Serial, Subject (not sure if an SP would have a subject or not)
The only common field among all three object types would be the Version field

To make all of this a bit clearer for myself, I made a table that might help to understand which fields are necessary for each object (with some fields that I'm not sure yet and some fields that are still missing):

Attribute		RPC	SP	PCR	SPT	SPRT	RCSR	PSR	RSR	Comment
Stored at MS		x	x	x	(x)	(x)				C1
Version			x	x	x	x	x	x	x	x	
Signature		x	x	x	x	x				
Issuer			x	x	x	(x)	(x)	x	x	x	C2
Serial			x	x	x			x	x	x	
Subject			x	(x)	x			x	(x)	x	C3
Public Key (+alg)	x					x			
Validity		?	?				?	?		C4
Policy			x							
LogID					x	x				
Timestamp		(x)	(x)	(x)	x	x				C5

Table comments:

1. object is included in other object (SPT in RPC or SP, SPRT in PCR)
2. we could consider the issuer as the LogID
3. the SP may not need a subject because it cannot be used to sign another policy object
4. not sure yet if RPCs and SPs have validity periods or not (they could simply have a timestamp and the object with the newest timestamp is considered valid). RPCs of PCAs (i.e., policy certificates with the CA bit set) would likely have a validity period set.
5. similar to C4, it is not clear if RPCs, SPs, and PCRs should have timestamps or not.

---

pkg/common/structure.go line 14 at r1 (raw file):

	__PolicyObjectMarkerMethod()
	Raw() []byte
	Domain() string

I'm not sure if all policy objects are really tied to a (single) domain.
For example the PCA certificate that is used to sign an RPC.
Should we also treat this as an RPC (maybe with a CA bit flag or without a domain) or as a completely different object?
Because if it is a policy object, then it would not have any domain as it can sign RPCs for any domain (or even sign intermediate certificates that don't have a domain).

---

pkg/common/structure.go line 22 at r1 (raw file):

}

func (PolicyObjectBase) __PolicyObjectMarkerMethod() {}

What does this function do?

---

pkg/common/structure.go line 180 at r1 (raw file):

func (sprt *SPRT) Equal(sprt_ *SPRT) bool {
	return true &&

why the true &&? Just for readability?

---

pkg/db/mysql/policies.go line 39 at r1 (raw file):

	str := "SELECT GROUP_CONCAT(presence SEPARATOR '') FROM (" +
		"SELECT (CASE WHEN policies.policy_id IS NOT NULL THEN 1 ELSE 0 END) AS presence FROM (" +
		strings.Join(elems, " UNION ALL ") +

Are you sure that this is guaranteed to work?
What if the UNION ALL operation does not append the rows in order (it seems that this is not guaranteed: https://dba.stackexchange.com/questions/316818/are-results-from-union-all-clauses-always-appended-in-order).

A possible solution would be to order the ids in the GO part first and then add an ORDER BY policy_id after the UNION ALL part, but I'm not sure about the performance impact for very large number of ids.

[juagargi]

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

---

pkg/common/structure.go line 10 at r1 (raw file):

Previously, cyrill-k wrote…

I'm not too happy with the currently used policy object names, as I think they are quite confusing.
I would suggest to rename them in a more consistent manner.
Should we do this in this PR or a separate one?

The renaming can happen in this PR. I don't love these names either, please propose some new ones 😃

---

pkg/common/structure.go line 14 at r1 (raw file):

Previously, cyrill-k wrote…

I'm not sure if all policy objects are really tied to a (single) domain.
For example the PCA certificate that is used to sign an RPC.
Should we also treat this as an RPC (maybe with a CA bit flag or without a domain) or as a completely different object?
Because if it is a policy object, then it would not have any domain as it can sign RPCs for any domain (or even sign intermediate certificates that don't have a domain).

This Domain function should be called Subject, as it is used as such.
It tries to mimic the behavior of web PKI, where even CA certificates have a subject, although it doesn't correspond to a domain name.
Would that renaming be enough? I.e. all policy objects have a subject represented by a string.

---

pkg/common/structure.go line 22 at r1 (raw file):

Previously, cyrill-k wrote…

What does this function do?

Its a marker method, only useful to mark some types as implementing certain interface.
It was useful when the PolicyObject had no other methods, but now it is not anymore.

---

pkg/common/structure.go line 180 at r1 (raw file):

Previously, cyrill-k wrote…

why the true &&? Just for readability?

Yes 😊
I don't know how to do it otherwise that makes the linter happy.
In the previous function it makes more sense than here. tbh; I will remove it from here but leave the other one.

---

pkg/db/mysql/policies.go line 39 at r1 (raw file):

Previously, cyrill-k wrote…

Are you sure that this is guaranteed to work?
What if the UNION ALL operation does not append the rows in order (it seems that this is not guaranteed: https://dba.stackexchange.com/questions/316818/are-results-from-union-all-clauses-always-appended-in-order).

A possible solution would be to order the ids in the GO part first and then add an ORDER BY policy_id after the UNION ALL part, but I'm not sure about the performance impact for very large number of ids.

That is a good catch. I checked the docs and MySQL does not warrant the order to be preserved even when using UNION ALL (or I couldn't find anywhere in their official docs where they would state it).

This is not the only place where we use a UNION or UNION ALL. But it is, together with its equivalent in the certs table, the only one that expects the order to be preserved. So this and certs.go has to change.
To avoid sorting, we'll indicate the DBE to sort by an already sorted column, in the hope that it will use some sorting algo that is very cheap when things are already sorted.

[cyrill-k]

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

---

pkg/common/structure.go line 14 at r1 (raw file):

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

This Domain function should be called Subject, as it is used as such.
It tries to mimic the behavior of web PKI, where even CA certificates have a subject, although it doesn't correspond to a domain name.
Would that renaming be enough? I.e. all policy objects have a subject represented by a string.

Yes, that should work, but then we need a way to distinguish between leaf (RPCs) and non-leaf (maybe RPCs?) certificates. If non-leafs are also RPC certificates, we would something like a CA bit (since we cannot do this via an empty Domain field anymore).

---

pkg/db/mysql/policies.go line 39 at r1 (raw file):

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

That is a good catch. I checked the docs and MySQL does not warrant the order to be preserved even when using UNION ALL (or I couldn't find anywhere in their official docs where they would state it).

This is not the only place where we use a UNION or UNION ALL. But it is, together with its equivalent in the certs table, the only one that expects the order to be preserved. So this and certs.go has to change.
To avoid sorting, we'll indicate the DBE to sort by an already sorted column, in the hope that it will use some sorting algo that is very cheap when things are already sorted.

That sounds good

[juagargi]

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

---

pkg/common/structure.go line 11 at r1 (raw file):

Previously, cyrill-k wrote…

I'm wondering whether it is the correct abstraction to group all of these objects together.
My suggestion is to group them as follows since they'll have more common fields that way:

1. Policy objects that can be identified (and thus returned) by the mapserver and thus the DB: RPC, SP, PCRevocation
2. Embedded policy objects: SPT, SPRT
3. Objects related to issuing policy objects, that are not relevant for the mapserver (but for the PCA): RCSR, PSR, Revocation Signing Request (doesn't exist yet and is maybe not needed)

The first two object types could be combined as signed policy objects and have the following common fields: Version, Issuer, Signature, SignatureAlgorithm
The first object type could then additionally have the common fields: Serial, Subject (not sure if an SP would have a subject or not)
The only common field among all three object types would be the Version field

To make all of this a bit clearer for myself, I made a table that might help to understand which fields are necessary for each object (with some fields that I'm not sure yet and some fields that are still missing):

Attribute		RPC	SP	PCR	SPT	SPRT	RCSR	PSR	RSR	Comment
Stored at MS		x	x	x	(x)	(x)				C1
Version			x	x	x	x	x	x	x	x	
Signature		x	x	x	x	x				
Issuer			x	x	x	(x)	(x)	x	x	x	C2
Serial			x	x	x			x	x	x	
Subject			x	(x)	x			x	(x)	x	C3
Public Key (+alg)	x					x			
Validity		?	?				?	?		C4
Policy			x							
LogID					x	x				
Timestamp		(x)	(x)	(x)	x	x				C5

Table comments:

1. object is included in other object (SPT in RPC or SP, SPRT in PCR)
2. we could consider the issuer as the LogID
3. the SP may not need a subject because it cannot be used to sign another policy object
4. not sure yet if RPCs and SPs have validity periods or not (they could simply have a timestamp and the object with the newest timestamp is considered valid). RPCs of PCAs (i.e., policy certificates with the CA bit set) would likely have a validity period set.
5. similar to C4, it is not clear if RPCs, SPs, and PCRs should have timestamps or not.

This table is super useful.
Fixed pretty much everything. There are still some things to take a look at:

• C4: I would assign validity to PCA RPCs, which are inherited by anything that is signed by them. Thoughts?
• C5: The timestamp would be the time when it was signed?
• C6: In SPT and SPRT the log ID is of type int. What is really the log ID? (related to C2)
• C7: In RPC and SP we have two signatures, one from CA, and the other one from who?
• C8: public key is only present in RPC and RCSR, but not in e.g. SP. What does this field represent?
• C9: The policy field in the table is present only in RPC, but in the code it's only in SP. Table is correct? What is an SP ?

---

pkg/common/structure.go line 14 at r1 (raw file):

Previously, cyrill-k wrote…

Yes, that should work, but then we need a way to distinguish between leaf (RPCs) and non-leaf (maybe RPCs?) certificates. If non-leafs are also RPC certificates, we would something like a CA bit (since we cannot do this via an empty Domain field anymore).

Done via IsCA attribute in RPC

[cyrill-k]

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

---

pkg/common/structure.go line 10 at r1 (raw file):

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

The renaming can happen in this PR. I don't love these names either, please propose some new ones 😃

New naming scheme:

• RPC + SP become PolicyCertificate, which has a subject, issuer, domain, IsIssuer bit, and PolicyAttributes.
  • IsIssuer specifies if this PolicyCertificate can be used to sign another certificate (or if it is a leaf). If the IsIssuer bit is not set, the PolicyCertificate may not have a public key.
  • domain specifies the domain that this PolicyCertificate is tied to. If this value is empty, then there is no restriction on the domain. If this value is non-empty, the domain attribute of any child must have the same value.
• RCSR + PSR become PolicyCertificateSigningRequest.
• SPT becomes SignedPolicyCertificateTimestamp.
• PolicyRevocation becomes PolicyCertificateRevocation.

---

pkg/common/structure.go line 11 at r1 (raw file):

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

This table is super useful.
Fixed pretty much everything. There are still some things to take a look at:

• C4: I would assign validity to PCA RPCs, which are inherited by anything that is signed by them. Thoughts?
• C5: The timestamp would be the time when it was signed?
• C6: In SPT and SPRT the log ID is of type int. What is really the log ID? (related to C2)
• C7: In RPC and SP we have two signatures, one from CA, and the other one from who?
• C8: public key is only present in RPC and RCSR, but not in e.g. SP. What does this field represent?
• C9: The policy field in the table is present only in RPC, but in the code it's only in SP. Table is correct? What is an SP ?

• C4: As discussed, for now, let's assume that validity periods are "inherited", i.e., the validity period of a child must be contained in the validity period of the parent.
• C5: Exactly, we need to decide which RPC+SP chain to use, we could for example take the chain with the newer timestamp.
• C6: In CT, this is the sha256 hash of the public key of the log (https://www.rfc-editor.org/rfc/rfc6962#section-3.2). So it should probably not be an int but a byte[32] instead.
• C7: The idea was that an RPC can only be updated with a signature of the previous RPC (to prevent a CA from hijacking a domain). And an SP can only be issued by the domain owner and thus requires a signature of the current RPC.
• C8: Since the SP is always a leaf, it does not need a public key.
• C9: Ahh, sorry that is wrong. The SP must have a policy. As discussed, an RPC should also be able to specify policy attributes, for example to restrict the power of a policy CA certificate or to specify company-wide policy attributes. See the updated table below:

Attribute		RPC	SP	PCR	SPT	SPRT	RCSR	PSR	RSR	Comment
Stored at MS		x	x	x	(x)	(x)				C1
Version			x	x	x	x	x	x	x	x	
Signature		x	x	x	x	x				
Issuer			x	x	x	(x)	(x)	x	x	x	C2
Serial			x	x	x			x	x	x	
Subject			x	x	x			x	(x)	x	C3
Public Key (+alg)	x					x			
Validity		?	?				?	?		C4
Policy			x	x							
LogID						x	x				
Timestamp		x	x	x	x	x				C5

---

pkg/common/structure.go line 14 at r1 (raw file):

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

Done via IsCA attribute in RPC

Let's use IsIssuer instead of IsCA.
See discussion on the policy object names.

[cyrill-k]

pkg/common/structure.go line 10 at r1 (raw file):

Previously, cyrill-k wrote…

New naming scheme:

• RPC + SP become PolicyCertificate, which has a subject, issuer, domain, IsIssuer bit, and PolicyAttributes.
  • IsIssuer specifies if this PolicyCertificate can be used to sign another certificate (or if it is a leaf). If the IsIssuer bit is not set, the PolicyCertificate may not have a public key.
  • domain specifies the domain that this PolicyCertificate is tied to. If this value is empty, then there is no restriction on the domain. If this value is non-empty, the domain attribute of any child must have the same value.
• RCSR + PSR become PolicyCertificateSigningRequest.
• SPT becomes SignedPolicyCertificateTimestamp.
• PolicyRevocation becomes PolicyCertificateRevocation.

I forgot two objects relating to revocations:

• SPRT becomes SignedPolicyCertificateRevocationTimestamp (same fields as the SignedPolicyCertificateTimestamp)
• RSR becomes PolicyCertificateRevocationSigningRequest

[juagargi]

Reviewable status: 0 of 157 files reviewed, all discussions resolved / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/structure.go line 10 at r1 (raw file):

Previously, cyrill-k wrote…

I forgot two objects relating to revocations:

• SPRT becomes SignedPolicyCertificateRevocationTimestamp (same fields as the SignedPolicyCertificateTimestamp)
• RSR becomes PolicyCertificateRevocationSigningRequest

done.

---

pkg/common/structure.go line 14 at r1 (raw file):

Previously, cyrill-k wrote…

Let's use IsIssuer instead of IsCA.
See discussion on the policy object names.

done.

[cyrill-k]

Reviewed 2 of 22 files at r4, 1 of 22 files at r5, 3 of 25 files at r6.
Reviewable status: 6 of 157 files reviewed, 9 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/embedded_policies.go line 22 at r6 (raw file):

	CertType        uint8     `json:",omitempty"`
	AddedTS         time.Time `json:",omitempty"`
	STH             []byte    `json:",omitempty"`

I think the naming does not really describe what this object represents.
In CT, the SCT is simply a timestamp, the logID, and the signature of the log over (pre)certificate+timestamp+certType.

Here, we already have the proof of inclusion (which may only be available after 24h)!
I think Yongzhe's idea was that we delay the ingestion of the policies until we have the proof of inclusion, then we don't need to worry about log servers not adding the policy as promised.

I see three ways to proceed here:

1. rename this into something more fitting, e.g., something like ProofOfInclusion but then we can remove the Signature (because the STH is signed and the signature is thus redundant), the AddedTS (because the STH contains a timestamp as well). I also think we would not need CertType anymore, since the PoI would simply point to a leaf in the log which would implicitly define the type.
2. actually change it into a signed timestamp by removing: STH, PoI, maybe STHSerialNumber, and maybe CertType (not sure if this is needed or not because we only have policy certificates for now).
3. Split this object into two objects and keep both and decide later if we only need one of them or both.

---

pkg/common/embedded_policies.go line 24 at r6 (raw file):

	STH             []byte    `json:",omitempty"`
	PoI             []byte    `json:",omitempty"`
	STHSerialNumber int       `json:",omitempty"`

What does this field represent?

---

pkg/common/policies.go line 31 at r6 (raw file):

type PolicyCertificateFields struct {
	PolicyCertificateBase

I think we're missing a Domain field, unless we assume that the Subject must be identical to the Domain.
But I think it is better to have two separate fields since they have different semantics:

• The subject identifies this object in a certificate chain, i.e., is used to find the issuer
• The domain restricts the namespace of the object

---

pkg/common/policies.go line 40 at r6 (raw file):

	TimeStamp          time.Time          `json:",omitempty"`
	PolicyAttributes   []PolicyAttributes `json:",omitempty"`
	OwnerSignature     []byte             `json:",omitempty"`

Do we need a way to identify the owner?
For example, assume there are multiple policy certificates for a domain which could be the owner that signed this certificate. Then the algorithm would have to check the signature with each of them because there is no way to identify the actual owner.

One solution is to have an Owner field that matches the Subject field of the owner certificate. This would not solve the problem if there are multiple certificates with the same Subject but that could be avoided by mandating to always have a new subject.

One way to solve this problem of having multiple possible parents with the same subject is to have something similar to the AuthorityKeyIdentifier and SubjectKeyIdentifier in X.509 (https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.1).
In X.509 there is no consistent format for these identifiers, but in our case we don't need any backward compatibility and we could simply define the SubjectKeyIdentifier as the SHA-256 hash of the public key. Then we would not need to add an explicit SubjectKeyIdentifier field because it can be calculated based on the public key. And each policy certificate could specify an IssuerKeyIdentifier and an OwnerKeyIdentifier.
What do you think about this?

---

pkg/common/policies.go line 46 at r6 (raw file):

type PolicyCertificate struct {
	PolicyCertificateFields
	IssuerSignature []byte                             `json:",omitempty"`

We could move the issuer signature to the PolicyDocument interface and PolicyCertificateBase struct since all PolicyDocuments are signed by an entity, i.e., they have an issuer.

Or is it better to keep it here since there are multiple signatures and thus it would not be clear which signature is from which entity?

I'm fine either way.

---

pkg/common/policies.go line 47 at r6 (raw file):

	PolicyCertificateFields
	IssuerSignature []byte                             `json:",omitempty"`
	SPTs            []SignedPolicyCertificateTimestamp `json:",omitempty"`

Should we use SPCTs to be consistent with the SPCRTs below?

---

pkg/common/policies.go line 80 at r6 (raw file):

	signatureAlgorithm SignatureAlgorithm,
	timeStamp time.Time,
	policyAttributes []PolicyAttributes,

I don't think we need an array of PolicyAttributes, because one PolicyAttributes object can already contain multiple attributes, i.e., both CA and subdomain restrictions.

---

pkg/common/policy_common.go line 10 at r6 (raw file):

// PolicyPart is an interface that is implemented by all objects that are part of the set
// of "policy objects". A policy object is that one that represents functionality of policies
// for a domain, such as RPC, RCSR, SPT, SPRT, SP, PSR or Policy.

We should update this to new policy object names.

---

pkg/common/policy_common.go line 19 at r6 (raw file):

	RawJSON []byte `json:"-"` // omit from JSON (un)marshaling
	Version int    `json:",omitempty"`
	Issuer  string `json:",omitempty"`

This means that PolicyCertificate(Revocation)Timestamp objects also have an Issuer.
In that case, what would it be set to? The url of the log? Or the logID, i.e., the hash of the log's public key?

[juagargi]

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

---

pkg/common/embedded_policies.go line 22 at r6 (raw file):

Previously, cyrill-k wrote…

I think the naming does not really describe what this object represents.
In CT, the SCT is simply a timestamp, the logID, and the signature of the log over (pre)certificate+timestamp+certType.

Here, we already have the proof of inclusion (which may only be available after 24h)!
I think Yongzhe's idea was that we delay the ingestion of the policies until we have the proof of inclusion, then we don't need to worry about log servers not adding the policy as promised.

I see three ways to proceed here:

1. rename this into something more fitting, e.g., something like ProofOfInclusion but then we can remove the Signature (because the STH is signed and the signature is thus redundant), the AddedTS (because the STH contains a timestamp as well). I also think we would not need CertType anymore, since the PoI would simply point to a leaf in the log which would implicitly define the type.
2. actually change it into a signed timestamp by removing: STH, PoI, maybe STHSerialNumber, and maybe CertType (not sure if this is needed or not because we only have policy certificates for now).
3. Split this object into two objects and keep both and decide later if we only need one of them or both.

Decided in a discussion offline that we take option # 2

---

pkg/common/embedded_policies.go line 24 at r6 (raw file):

Previously, cyrill-k wrote…

What does this field represent?

gone.

---

pkg/common/policies.go line 31 at r6 (raw file):

Previously, cyrill-k wrote…

I think we're missing a Domain field, unless we assume that the Subject must be identical to the Domain.
But I think it is better to have two separate fields since they have different semantics:

• The subject identifies this object in a certificate chain, i.e., is used to find the issuer
• The domain restricts the namespace of the object

done.

---

pkg/common/policies.go line 40 at r6 (raw file):

Previously, cyrill-k wrote…

Do we need a way to identify the owner?
For example, assume there are multiple policy certificates for a domain which could be the owner that signed this certificate. Then the algorithm would have to check the signature with each of them because there is no way to identify the actual owner.

One solution is to have an Owner field that matches the Subject field of the owner certificate. This would not solve the problem if there are multiple certificates with the same Subject but that could be avoided by mandating to always have a new subject.

One way to solve this problem of having multiple possible parents with the same subject is to have something similar to the AuthorityKeyIdentifier and SubjectKeyIdentifier in X.509 (https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.1).
In X.509 there is no consistent format for these identifiers, but in our case we don't need any backward compatibility and we could simply define the SubjectKeyIdentifier as the SHA-256 hash of the public key. Then we would not need to add an explicit SubjectKeyIdentifier field because it can be calculated based on the public key. And each policy certificate could specify an IssuerKeyIdentifier and an OwnerKeyIdentifier.
What do you think about this?

We are now identifying the owner and the issuer with OwnerPubKeyHash and IssuerPubKeyHash.

---

pkg/common/policies.go line 47 at r6 (raw file):

Previously, cyrill-k wrote…

Should we use SPCTs to be consistent with the SPCRTs below?

done

---

pkg/common/policies.go line 80 at r6 (raw file):

Previously, cyrill-k wrote…

I don't think we need an array of PolicyAttributes, because one PolicyAttributes object can already contain multiple attributes, i.e., both CA and subdomain restrictions.

done

---

pkg/common/policy_common.go line 10 at r6 (raw file):

Previously, cyrill-k wrote…

We should update this to new policy object names.

done

---

pkg/common/policy_common.go line 19 at r6 (raw file):

Previously, cyrill-k wrote…

This means that PolicyCertificate(Revocation)Timestamp objects also have an Issuer.
In that case, what would it be set to? The url of the log? Or the logID, i.e., the hash of the log's public key?

This is a good question...

[juagargi]

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

---

pkg/common/policies.go line 46 at r6 (raw file):

Previously, cyrill-k wrote…

We could move the issuer signature to the PolicyDocument interface and PolicyCertificateBase struct since all PolicyDocuments are signed by an entity, i.e., they have an issuer.

Or is it better to keep it here since there are multiple signatures and thus it would not be clear which signature is from which entity?

I'm fine either way.

This is the class diagram for those types:

If we add IssuerSignature there, also the PolicyCertificateSigningRequest would include it, which is not what we want.

[juagargi]

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

---

pkg/common/policy_common.go line 19 at r6 (raw file):

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

This is a good question...

Would the Issuer be the Subject of the certificate of the log? Should I set it that way in the pca tests to keeps the workflow documented there?

[cyrill-k]

Reviewed 5 of 25 files at r7.
Reviewable status: 7 of 162 files reviewed, 6 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/embedded_policies.go line 32 at r8 (raw file):

type SignedPolicyCertificateRevocationTimestamp struct {
	SignedEntryTimestamp
	Reason int `json:",omitempty"`

Why do we only have the reason for the revocation and not for the certificate?

---

pkg/common/policies.go line 46 at r6 (raw file):

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

This is the class diagram for those types:

If we add IssuerSignature there, also the PolicyCertificateSigningRequest would include it, which is not what we want.

That makes sense 👍

---

pkg/common/policies.go line 18 at r8 (raw file):

type PolicyCertificateBase struct {
	PolicyPartBase
	RawSubject      string `json:"Subject,omitempty"`

Does it still make sense to call this RawSubject, since we currently don't have any particular encoding and the issuer field in PolicyPartBase is also not called RawIssuer (just Issuer)?

---

pkg/common/policies.go line 42 at r8 (raw file):

	PolicyAttributes   PolicyAttributes   `json:",omitempty"`
	OwnerSignature     []byte             `json:",omitempty"`
	OwnerPubKeyHash    []byte             `json:",omitempty"` // SHA256 of owner's public key

I think we are missing an Owner field indicating the subject name of the owner certificate, i.e., analogous to the Issuer field for the IssuerSignature.

---

pkg/common/policy_common.go line 19 at r6 (raw file):

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

Would the Issuer be the Subject of the certificate of the log? Should I set it that way in the pca tests to keeps the workflow documented there?

Do log servers have a certificate or not? AFAIK, in CT, they are simply identified by their public key and don't have any certificate, e.g., there is no X.509 certificate of a log...

We could either (1) move theIssuer field to the PolicyCertificateBase struct since we also have the subject field there (i.e., the relevant corresponding field) or (2) modify NewSignedPolicyCertificateRevocationTimestamp to not accept an issuer parameter and set the Issuer field to, for example, nil or the base64 encoding of the LogID...

---

pkg/common/policy_issuance.go line 31 at r8 (raw file):

	policyAttributes PolicyAttributes,
	ownerSignature []byte,
	ownerPubKeyHash []byte,

I think we need an Owner field (see comment in policies.go)

[juagargi]

Reviewable status: 7 of 162 files reviewed, 5 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/embedded_policies.go line 32 at r8 (raw file):

Previously, cyrill-k wrote…

Why do we only have the reason for the revocation and not for the certificate?

I really don't know. I took the field from the existing code; it wasn't in the certificate revocation.
Additionally, should it be an int or something else? What's the catalog of different reasons?

---

pkg/common/policies.go line 18 at r8 (raw file):

Previously, cyrill-k wrote…

Does it still make sense to call this RawSubject, since we currently don't have any particular encoding and the issuer field in PolicyPartBase is also not called RawIssuer (just Issuer)?

The name RawSubject exists to differentiate from Subject, which is a method in the interface that returns the subject. The field could be called SubjectField, or the Subject() method renamed to GetSubject() if we think it helps.

---

pkg/common/policies.go line 42 at r8 (raw file):

Previously, cyrill-k wrote…

I think we are missing an Owner field indicating the subject name of the owner certificate, i.e., analogous to the Issuer field for the IssuerSignature.

I thought we agreed to not use the identification of the issuer or owner via Issuer or Owner.
In my opinion, we should remove the fields Issuer and Subject, and always identify issuer and owner via the hash of something that uniquely identifies the certificate (payload maybe?).
We could add a new field Description, if people would want to have a human readable description of the certificate and what it is used for.

---

pkg/common/policy_common.go line 19 at r6 (raw file):

Previously, cyrill-k wrote…

Do log servers have a certificate or not? AFAIK, in CT, they are simply identified by their public key and don't have any certificate, e.g., there is no X.509 certificate of a log...

We could either (1) move theIssuer field to the PolicyCertificateBase struct since we also have the subject field there (i.e., the relevant corresponding field) or (2) modify NewSignedPolicyCertificateRevocationTimestamp to not accept an issuer parameter and set the Issuer field to, for example, nil or the base64 encoding of the LogID...

I'd vote to not have certificates for the CT log servers. We can follow the same workflow web pki has.
Before I start moving around the Issuer field, let's solve the question in another comment about completely removing it or not.

---

pkg/common/policy_issuance.go line 31 at r8 (raw file):

Previously, cyrill-k wrote…

I think we need an Owner field (see comment in policies.go)

Yes, let's tie the destiny of Owner to that of Issuer. If Issuer is removed, we won't need an Owner, if we keep it, we'll add an Owner.

[cyrill-k]

pkg/common/policies.go line 42 at r8 (raw file):

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

I thought we agreed to not use the identification of the issuer or owner via Issuer or Owner.
In my opinion, we should remove the fields Issuer and Subject, and always identify issuer and owner via the hash of something that uniquely identifies the certificate (payload maybe?).
We could add a new field Description, if people would want to have a human readable description of the certificate and what it is used for.

Let's use a new field IssuerHash which is the hash of the payload without SPCTs and without issuer signature while preserving the owner signature.

The analogous is done for the OwnerHash field.

And remove the Issuer and Subject field.

[juagargi]

pkg/common/policies.go line 42 at r8 (raw file):

Previously, cyrill-k wrote…

Let's use a new field IssuerHash which is the hash of the payload without SPCTs and without issuer signature while preserving the owner signature.

The analogous is done for the OwnerHash field.

And remove the Issuer and Subject field.

Note for future self: the issuer will not be present in the SPCTs or requests, only in certificate and revocation.
Also, we can remove now the IssuerPubKeyHash and OwnerPubKeyHash.

[juagargi]

Reviewable status: 3 of 168 files reviewed, all discussions resolved / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/embedded_policies.go line 32 at r8 (raw file):

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

I really don't know. I took the field from the existing code; it wasn't in the certificate revocation.
Additionally, should it be an int or something else? What's the catalog of different reasons?

Gone.

---

pkg/common/policies.go line 18 at r8 (raw file):

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

The name RawSubject exists to differentiate from Subject, which is a method in the interface that returns the subject. The field could be called SubjectField, or the Subject() method renamed to GetSubject() if we think it helps.

Renamed the RawFoo fields to FooField.

---

pkg/common/policies.go line 42 at r8 (raw file):

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

Note for future self: the issuer will not be present in the SPCTs or requests, only in certificate and revocation.
Also, we can remove now the IssuerPubKeyHash and OwnerPubKeyHash.

Done.

---

pkg/common/policy_common.go line 19 at r6 (raw file):

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

I'd vote to not have certificates for the CT log servers. We can follow the same workflow web pki has.
Before I start moving around the Issuer field, let's solve the question in another comment about completely removing it or not.

Done.

---

pkg/common/policy_issuance.go line 31 at r8 (raw file):

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

Yes, let's tie the destiny of Owner to that of Issuer. If Issuer is removed, we won't need an Owner, if we keep it, we'll add an Owner.

We have an OwnerHash

[cyrill-k]

Reviewed 6 of 29 files at r9.
Reviewable status: 9 of 168 files reviewed, 7 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/json.go line 71 at r9 (raw file):

		T = "spct"
	case PolicyCertificateRevocation:
		T = "pcrev"

Minor point, but should we call it pcr to be consistent with the variable naming in the other policy files?

---

pkg/common/json.go line 73 at r9 (raw file):

		T = "pcrev"
	case SignedPolicyCertificateRevocationTimestamp:
		T = "spcrevt"

Same as above, should we use spcrt instead?

---

pkg/common/json.go line 77 at r9 (raw file):

		T = "trillian.Proof"
	case trilliantypes.LogRootV1:
		T = "logrootv1"

Should we also add an identifier for PolicyCertificateRevocationSigningRequest, i.e., pcrsr?

---

pkg/common/policies.go line 60 at r9 (raw file):

// a previously existing policy certificate.
// The field `IssuerHash` has semantics analogouys to `OwnerHash`: it is the SHA256 of the issuer
// policy certificate that was used to sign this policy certificate, without SCPTs.

Also without the IssuerSignature, right?
Since the original IssuerSignature value (before issuing the SPCTs) cannot be reconstructed by the relying party. And the IssuerSignature value might be different depending on which SPCTs are embedded.

---

pkg/common/policies.go line 84 at r9 (raw file):

	PolicyCertificateRevocationFields
	IssuerSignature []byte `json:",omitempty"`
	// Hash of the issuer's cert w/out SPCTs:

Same as above, this should also be without the IssuerSignature.

---

pkg/common/policies.go line 225 at r9 (raw file):

	ownerSignature []byte,
	ownerHash []byte,
	serverTimestamps []SignedPolicyCertificateRevocationTimestamp,

Maybe we can rename them to SPCRTs, analogous to SPCTs in NewPolicyCertificate?

---

pkg/common/policy_issuance.go line 13 at r9 (raw file):

type PolicyCertificateRevocationSigningRequest struct {
	PolicyCertificateHash []byte `json:",omitempty"` // Hash of the pol. cert. to revoke

again excluding any SPCTs and IssuerSignature, right?

[juagargi]

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

---

pkg/common/json.go line 71 at r9 (raw file):

Previously, cyrill-k wrote…

Minor point, but should we call it pcr to be consistent with the variable naming in the other policy files?

The rev suffix is there to kind of distinguish it from the r in Request. Minor thing nevertheless, let me know if you hate pcrev or spcrevt and I'll change it.

---

pkg/common/json.go line 73 at r9 (raw file):

Previously, cyrill-k wrote…

Same as above, should we use spcrt instead?

See previous comment.

---

pkg/common/json.go line 77 at r9 (raw file):

Previously, cyrill-k wrote…

Should we also add an identifier for PolicyCertificateRevocationSigningRequest, i.e., pcrsr?

We can. I have so far refrained from doing anything regarding the revocation request, but if you see it necessary now, we would also have to add a process into the PCA and DomainOwner classes (types).
For now I am only adding the (de)serialization code here.

---

pkg/common/policies.go line 60 at r9 (raw file):

Previously, cyrill-k wrote…

Also without the IssuerSignature, right?
Since the original IssuerSignature value (before issuing the SPCTs) cannot be reconstructed by the relying party. And the IssuerSignature value might be different depending on which SPCTs are embedded.

The analogous to OwnerHash in this case would preserve the IssuerSignature, as the OwnerHash preserves the OwnerSignature.

I do agree that it is a problem to remove the SPCTs but keep the IssuerSignature to compute.
Aaaaand I forgot why we had to remove the SPCTs in the first place. Why was the reason? Why couldn't any other party obtain the exact identical policy certificate that issued this child certificate? (with SPCTs and all)

[cyrill-k]

Reviewed 4 of 6 files at r10.
Reviewable status: 8 of 168 files reviewed, 4 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/json.go line 71 at r9 (raw file):

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

The rev suffix is there to kind of distinguish it from the r in Request. Minor thing nevertheless, let me know if you hate pcrev or spcrevt and I'll change it.

no, that's okay for me 👍

---

pkg/common/json.go line 77 at r9 (raw file):

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

We can. I have so far refrained from doing anything regarding the revocation request, but if you see it necessary now, we would also have to add a process into the PCA and DomainOwner classes (types).
For now I am only adding the (de)serialization code here.

I think that's fine, we can do the actual implementation later, but I think defining the format is a good thing to do now already.

---

pkg/common/policies.go line 60 at r9 (raw file):

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

The analogous to OwnerHash in this case would preserve the IssuerSignature, as the OwnerHash preserves the OwnerSignature.

I do agree that it is a problem to remove the SPCTs but keep the IssuerSignature to compute.
Aaaaand I forgot why we had to remove the SPCTs in the first place. Why was the reason? Why couldn't any other party obtain the exact identical policy certificate that issued this child certificate? (with SPCTs and all)

If we keep the SPCTs in the hash computation, the parent policy certificate would only be valid with exactly these SPCTs and this is not desired since the SPCTs only indicate that the precertificate is logged in their database.

Please take a look at the UF-PKI paper (section IV B) to see if the argument written there makes sense to you (second paragraph).

[juagargi]

Reviewable status: 8 of 168 files reviewed, all discussions resolved / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

pkg/common/json.go line 77 at r9 (raw file):

Previously, cyrill-k wrote…

I think that's fine, we can do the actual implementation later, but I think defining the format is a good thing to do now already.

done

---

pkg/common/policies.go line 60 at r9 (raw file):

Previously, cyrill-k wrote…

If we keep the SPCTs in the hash computation, the parent policy certificate would only be valid with exactly these SPCTs and this is not desired since the SPCTs only indicate that the precertificate is logged in their database.

Please take a look at the UF-PKI paper (section IV B) to see if the argument written there makes sense to you (second paragraph).

done. After another offline discussion we agree that the hash has to be SPCT independent, and thus the IssuerSignature has to go away.

---

pkg/common/policy_issuance.go line 13 at r9 (raw file):

Previously, cyrill-k wrote…

again excluding any SPCTs and IssuerSignature, right?

yes, because we intend to revoke any certificate representation that contained that certificate data. It won't matter if they payload had X or Y SPCTs.

[cyrill-k]

Reviewed 1 of 21 files at r3, 1 of 6 files at r10, 2 of 17 files at r11.
Reviewable status: 10 of 176 files reviewed, all discussions resolved / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

[cyrill-k]

Reviewed 13 of 13 files at r12, all commit messages.
Reviewable status: 23 of 176 files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

tools/create_schema.sh line 11 at r12 (raw file):

MYSQLCMD="mysql -u root"

variable defined twice?

[cyrill-k]

Reviewed 1 of 1 files at r13.
Reviewable status: 23 of 176 files reviewed, all discussions resolved / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

[cyrill-k]

Reviewed 105 of 152 files at r1, 7 of 21 files at r3, 1 of 22 files at r4, 1 of 22 files at r5, 3 of 25 files at r6, 8 of 29 files at r7, 1 of 1 files at r8, 12 of 29 files at r9, 15 of 17 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @cyrill-k)