Create Hasher Factory #365
Conversation
gdbelvin
force-pushed
the
testhasher
branch
2 times, most recently
from
1f3b2a8
to
9892f15
Compare
|
Did this get rebased after #364? |
|
This is rebased and ready for review |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package rfc6962 |
There was a problem hiding this comment.
Arguably this should be part of the personality specific code but I don't think we can do this without having a dependency from generic -> personality that we don't want.
There was a problem hiding this comment.
I agree. There's a lot of places in the code where this is either hard-coded, supposed to be looked up from the log config.
Hopefully this change to putting this hashing algorithm in it's own package will make it easier to isolate the personality specific stuff later.
server/proof_fetcher.go
Outdated
| // TODO(Martin2112): Hasher must be selected based on log config. | ||
| hasher, err := merkle.Factory("RFC6962-SHA256") | ||
| if err != nil { | ||
| panic("Uknown hash strategy") |
storage/mysql/log_storage.go
Outdated
| @@ -160,15 +160,23 @@ func (t *readOnlyLogTX) GetActiveLogIDsWithPendingWork() ([]int64, error) { | |||
| return getActiveLogIDsWithPendingWork(t.tx) | |||
| } | |||
|
|
|||
| func (m *mySQLLogStorage) hasher(treeID int64) (merkle.TreeHasher, error) { | |||
| // TODO: read hash algorithm from storage. | |||
| return merkle.Factory("RFC6962-SHA256") | |||
There was a problem hiding this comment.
I'm thinking we should declare these hasher type strings as constants then IDEs can find usages of them easily.
| ) | ||
|
|
||
| // Hasher is the default hasher for tests. | ||
| // TODO: Make this a custom algorithm to decouple hashing from coded defaults. |
There was a problem hiding this comment.
Can we do this soon after this PR? It would show up if there's any RFC6962 specific assumptions in the generic code. There shouldn't be but it's possible.
There was a problem hiding this comment.
I don't think we can do this very soon. Doing this means re-writing the tests such that they don't depend on test vectors that are specific to this hash function. Anyone want to volunteer?
merkle/tree_hasher.go
Outdated
| HashEmpty() []byte | ||
| HashLeaf(leaf []byte) []byte | ||
| HashChildren(l, r []byte) []byte | ||
| Size() int |
There was a problem hiding this comment.
I don't like Size() for the name of this method but that's what the Go crypto uses so we can leave it like this.
There was a problem hiding this comment.
In a future PR we can rename this BitLength() and return the number of bits in the hash to as to remove many of the Size()*8 bits I see in the code.
merkle/hstar2_test.go
Outdated
| @@ -27,7 +27,7 @@ import ( | |||
|
|
|||
| // This root was calculated with the C++/Python sparse Merkle tree code in the | |||
| // github.com/google/certificate-transparency repo. | |||
| const sparseEmptyRootHashB64 = "xmifEIEqCYCXbZUz2Dh1KCFmFZVn7DUVVxbBQTr1PWo=" | |||
| var sparseEmptyRootHashB64 = testonly.MustDecodeBase64("xmifEIEqCYCXbZUz2Dh1KCFmFZVn7DUVVxbBQTr1PWo=") | |||
There was a problem hiding this comment.
Is this going to break if you change the testonly hasher? Add a TODO if so.
There was a problem hiding this comment.
Please clarify?
- This doesn't change the behavior of the test as is - so it passes.
- If we changed the test only hasher, yes, this would break. A stronger test would compute this properly based on the hasher, but there's not enough documentation on how the test vector was computed to do so.
There was a problem hiding this comment.
I just meant to TODO it to switch this to a generic test.
- Support looking up the hasher based on the log config. - Create a separate, test hasher for use in tests. This supports future decoupling of tests from hard-coded values. - Moves RFC6962 hasher into its own module.
This supports future decoupling of tests from hard-coded values.
Partial work towards #331