Check for duplicate certs before adding to db

[andygabby]

Error at SA if the certificate or precertificate already exist in the
database

Fixes: #5468

[aarongable]

I think it's simpler (and maybe faster?) to do:

Suggested change

	var rows []interface{}
	results, err := txWithCtx.Select(&rows, "SELECT id FROM precertificates WHERE serial=?", serialHex)
	c, err := txWithCtx.SelectInt("SELECT count(id) FROM precertificates WHERE serial=?", serialHex)

and then compare c > 0 instead of len(results) > 0.

[beautifulentropy]

From the gorp docs:

SelectInt executes the given query, which should be a SELECT statement for a single integer column, and returns the value of the first row returned. If no rows are found, zero is returned.

So using 0 as a sentinel would be c == 0.

Alternatively, you can do a SelectOne() and your sentinel would be: errors.Is(err, sql.ErrNoRows) but you'll also have to check for fmt.Errorf("gorp: multiple rows returned for: %s - %v", query, args) so the SelectInt() is probably the more elegant solution.

If you use a SelectOne(), make a composite type with an exported field of ID int64 and pass a slice of that composite type as the holder.

[aarongable]

Note that my suggestion is to select count(id), not just id, so we're not looking for a sentinel here -- we're literally just looking for "how many rows would you have returned?", and if the answer is anything greater than zero, then we have a problem.

[beautifulentropy]

Note that my suggestion is to select count(id), not just id, so we're not looking for a sentinel here -- we're literally just looking for "how many rows would you have returned?", and if the answer is anything greater than zero, then we have a problem.

Ah, I missed that.

[jsha]

I agree with @aarongable's proposal in concept, but there's a catch: we use the db.Executor interface here, which is a subset of the methods generally available on gorp.DbMap. It includes Select and SelectOne, but it doesn't include SelectInt. I think it's probably not worth plumbing through SelectInt (and making sure it's implemented in all relevant DB mocks) for this small optimization. What do you think, @aarongable ?

[aarongable]

Gah, you're probably right that it isn't worthwhile "just" for this, but I do see a bunch of other places where we could use it. In particular, there are a bunch of places where we use SelectOne(&count, ...) in order to get a single integer out of the database. Which is also what we should do here, if we don't plumb SelectInt all the way through the wrapper.

[beautifulentropy]

From the gorp docs:

SelectInt executes the given query, which should be a SELECT statement for a single integer column, and returns the value of the first row returned. If no rows are found, zero is returned.

So using 0 as a sentinel would be c == 0.

Alternatively, you can do a SelectOne() and your sentinel would be: errors.Is(err, sql.ErrNoRows) but you'll also have to check for fmt.Errorf("gorp: multiple rows returned for: %s - %v", query, args) so the SelectInt() is probably the more elegant solution.

If you use a SelectOne(), make a composite type with an exported field of ID int64 and pass a slice of that composite type as the holder.

[jsha]

This looks good as-is. As a followup, it would be nice to add an extra layer of enforcement for the property I mentioned in #5467 (comment): that anytime we are treating rows as duplicates in this table it is because they are exact byte-for-byte DER duplicates.

What I imagine that looking like here is changing the SelectOne to also get the DER. We would then compare that to the DER we're trying to insert. If it's the same, we can happily return DuplicateError. Otherwise, we should go ahead with the insert (to ensure we have a copy for investigation), and audit log the problem. Alternately, we could return some error other than DuplicateError, which would cause the CA to keep the certificate in its orphan queue and keep retrying, which would eventually show up in the metrics.