Opened 8 months ago

Closed 6 months ago

#29108 closed enhancement (implemented)

Refactor crypto_digest.c to have fewer ifdefs

Reported by: nickm Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: nickm-merge, asn-merge, 041-proposed, refactor, technical-debt
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: catalyst Sponsor: Sponsor31-can

Description

Our current crypto_digest.c is a maze of twisty little ifdefs, and OpenSSL keccak support in #28837 will make it even more so. We ought to refactor it so that it's less idiosyncratic.

Possibly we should take the same approach as in the other crypto_ops modules, and divide it into an OpenSSL portion, a NSS portion, and a common portion.

Child Tickets

Change History (11)

comment:1 Changed 8 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:2 Changed 8 months ago by rl1987

Status: acceptedneeds_review

https://github.com/torproject/tor/pull/657

I could try unifying this patch with #28837, if that's needed.

comment:3 Changed 8 months ago by dgoulet

Reviewer: catalyst

comment:4 in reply to:  2 Changed 7 months ago by catalyst

Status: needs_reviewneeds_revision

Replying to rl1987:

https://github.com/torproject/tor/pull/657

I could try unifying this patch with #28837, if that's needed.

Thanks, if you could please resolve the current merge conflict, that would be great! Feel free to force-push the pull request branch this time.

comment:5 Changed 7 months ago by rl1987

Status: needs_revisionneeds_review

comment:6 in reply to:  5 Changed 6 months ago by catalyst

Replying to rl1987:

https://github.com/torproject/tor/pull/752

This had a stochastic test failure so I restarted that Travis job and will try to look at it if that passes.

comment:7 in reply to:  5 Changed 6 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to rl1987:

https://github.com/torproject/tor/pull/752

Thanks! This looks good. I did manual builds with NSS and OpenSSL.

comment:8 Changed 6 months ago by teor

Keywords: nickm-merge asn-merge teor-merge added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Type: taskenhancement

This is a refactor, so it belongs in master.
(We also have NSS in our Travis CI, and it passed.)

comment:9 Changed 6 months ago by nickm

Keywords: teor-merge removed

batch-modify: asn has offered to merge these tomorrow, so removing them from teor's plate. Teor -- you can merge these anyway if you are blocked on any of them and it would save you time to do so.

comment:10 Changed 6 months ago by nickm

color-move makes the double-checking pretty nice here. Merged to master!

comment:11 Changed 6 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.