Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: ber to der convertor #176

Closed
wants to merge 17 commits into from
Closed

feat: ber to der convertor #176

wants to merge 17 commits into from

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Dec 7, 2023 •
edited
Loading

Migrated the PR to tspclient-go repo: notaryproject/tspclient-go#11

Feat:

  • added BER to DER convertor

Test:

  • added part of the unit test cases; will complete them after code review

Signed-off-by: Junjie Gao [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2023 •
edited
Loading

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (9038355) 89.32% compared to head (7698ae3) 90.17%.

Files Patch % Lines
internal/crypto/cms/encoding/ber/ber.go 96.38% 4 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
+ Coverage   89.32%   90.17%   +0.84%     
==========================================
  Files          21       25       +4     
  Lines        1724     1934     +210     
==========================================
+ Hits         1540     1744     +204     
- Misses        149      153       +4     
- Partials       35       37       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@priteshbandi
Copy link
Contributor

Is there any existing reliable timestamping (RFC 3161) library that we can use instead of creating and maintaining new one?

@shizhMSFT
Copy link
Contributor

Is there any existing reliable timestamping (RFC 3161) library that we can use instead of creating and maintaining new one?

Unfortunately, no for golang.

@shizhMSFT shizhMSFT changed the title feat: BER to DER convertor feat: ASN.1 BER to DER convertor Dec 8, 2023
@shizhMSFT
Copy link
Contributor

@priteshbandi You may take a look at and assess digitorus/timestamp, which is based on digitorus/pkcs7.

Two-Hearts
Two-Hearts reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished 1st round of review: BER decoding
mainly, refactored a bit of the DFS logic to improve readability.

internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
Two-Hearts
Two-Hearts reviewed Dec 22, 2023
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2nd round of review, the overall logic looks good to me.

internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/asn1.go Outdated Show resolved Hide resolved
internal/encoding/asn1/primitive.go Outdated Show resolved Hide resolved
internal/encoding/asn1/constructed.go Outdated Show resolved Hide resolved
internal/encoding/asn1/common.go Outdated Show resolved Hide resolved
internal/encoding/asn1/common.go Outdated Show resolved Hide resolved
internal/encoding/asn1/constructed.go Outdated Show resolved Hide resolved
@JeyJeyGao JeyJeyGao changed the title feat: ASN.1 BER to DER convertor feat: cms package Dec 26, 2023
Two-Hearts
Two-Hearts reviewed Dec 26, 2023
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please separate package cms to a new PR which depends on this PR. @JeyJeyGao

@JeyJeyGao JeyJeyGao changed the title feat: cms package feat: ber to der convertor Dec 26, 2023
@JeyJeyGao JeyJeyGao mentioned this pull request Dec 26, 2023
Closed
@JeyJeyGao
Copy link
Contributor Author

Please separate package cms to a new PR which depends on this PR. @JeyJeyGao

created #181 for cms

@JeyJeyGao
Squashed commit of the following:
69d5c49 
commit 9240650
Merge: 0c1ec3b 4198690
Author: Junjie Gao <[email protected]>
Date:   Wed Aug 9 17:07:34 2023 +0800

    Merge pull request #1 from JeyJeyGao/feat/ans1

    feat: convert BER to DER

commit 4198690
Author: Junjie Gao <[email protected]>
Date:   Wed Aug 9 09:14:29 2023 +0800

    fix: simplify code

    Signed-off-by: Junjie Gao <[email protected]>

commit 75ce02d
Author: Junjie Gao <[email protected]>
Date:   Mon Aug 7 20:33:08 2023 +0800

    fix: added Conetent method for value interface

    Signed-off-by: Junjie Gao <[email protected]>

commit 7b823a9
Author: Junjie Gao <[email protected]>
Date:   Mon Aug 7 08:54:37 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit 41ecec6
Author: Junjie Gao <[email protected]>
Date:   Sun Aug 6 17:33:19 2023 +0800

    fix: remove recusive call for encode()

    Signed-off-by: Junjie Gao <[email protected]>

commit 8f1a2af
Author: Junjie Gao <[email protected]>
Date:   Fri Aug 4 13:40:09 2023 +0800

    fix: remove unused value

    Signed-off-by: Junjie Gao <[email protected]>

commit 9b6a0c5
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 20:25:22 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit 91a3691
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 20:11:28 2023 +0800

    fix: create pointer instead of value to improve performance

    Signed-off-by: Junjie Gao <[email protected]>

commit 1465e3e
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 20:04:44 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit 6524a9c
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 19:53:27 2023 +0800

    fix: update variable naming

    Signed-off-by: Junjie Gao <[email protected]>

commit 6cfbd9c
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 19:47:39 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit b9c73bd
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 17:56:52 2023 +0800

    fix: update to use rawContent instead of expectedLen

    Signed-off-by: Junjie Gao <[email protected]>

commit 3c99402
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 16:45:09 2023 +0800

    fix: update comment

    Signed-off-by: Junjie Gao <[email protected]>

commit f4dc95f
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 16:41:57 2023 +0800

    fix: resolve comment

    Signed-off-by: Junjie Gao <[email protected]>

commit f916316
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 16:40:37 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit 22afdf8
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 16:34:34 2023 +0800

    fix: resolve comment

    Signed-off-by: Junjie Gao <[email protected]>

commit edb729c
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 16:32:47 2023 +0800

    fix: resolve comment

    Signed-off-by: Junjie Gao <[email protected]>

commit a8ba0ff
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 16:26:29 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit bc18cae
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 16:14:57 2023 +0800

    fix: resolve comments

    Signed-off-by: Junjie Gao <[email protected]>

commit 643f388
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 09:17:39 2023 +0800

    fix: update comment

    Signed-off-by: Junjie Gao <[email protected]>

commit b5d5131
Author: Junjie Gao <[email protected]>
Date:   Thu Aug 3 09:15:23 2023 +0800

    fix: expectedLen == 0 should continue

    Signed-off-by: Junjie Gao <[email protected]>

commit 2345740
Author: Junjie Gao <[email protected]>
Date:   Wed Aug 2 13:01:38 2023 +0800

    fix: added copyright

    Signed-off-by: Junjie Gao <[email protected]>

commit 936ba2b
Author: Junjie Gao <[email protected]>
Date:   Wed Aug 2 11:36:02 2023 +0800

    fix: remove recusive decoding

    Signed-off-by: Junjie Gao <[email protected]>

commit 4fd944a
Author: Junjie Gao <[email protected]>
Date:   Tue Aug 1 21:50:10 2023 +0800

    fix: remove readOnlySlice

    Signed-off-by: Junjie Gao <[email protected]>

commit efa7575
Author: Junjie Gao <[email protected]>
Date:   Tue Aug 1 09:38:57 2023 +0800

    fix: update decodeIdentifier function name

    Signed-off-by: Junjie Gao <[email protected]>

commit cbce4c1
Author: Junjie Gao <[email protected]>
Date:   Tue Aug 1 09:25:34 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit 45480e5
Author: Junjie Gao <[email protected]>
Date:   Mon Jul 31 21:22:20 2023 +0800

    fix: update code

    Signed-off-by: Junjie Gao <[email protected]>

commit b3de155
Author: Junjie Gao <[email protected]>
Date:   Mon Jul 31 20:51:48 2023 +0800

    fix: set non-exportable type

    Signed-off-by: Junjie Gao <[email protected]>

commit 5dea9e5
Author: Junjie Gao <[email protected]>
Date:   Mon Jul 31 20:44:50 2023 +0800

    feat: asn.1 first version

    Signed-off-by: Junjie Gao <[email protected]>

Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
test: add unit test
aebb36d 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: add license
afe0aec 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: optimize test
65bc051 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update test
9f6e6b0 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update comment
148e0a9 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: resolve comments
49dd7a6 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: resolve comments
aee8248 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update
bc72eb6 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: update
9e8a32e 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: remove common errors
548c448 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
feat: added cms
5614972 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: remove cms
5854500 
Signed-off-by: Junjie Gao <[email protected]>
Two-Hearts
Two-Hearts reviewed Dec 28, 2023
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the last round of my review. Overall LGTM.

internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
internal/crypto/cms/encoding/ber/ber.go Outdated Show resolved Hide resolved
@JeyJeyGao
fix: resolve comment
1b83aa0 
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao
fix: test
43f94bb 
Signed-off-by: Junjie Gao <[email protected]>
Two-Hearts
Two-Hearts previously approved these changes Jan 2, 2024
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please create an issue to track adding unit tests to the new lib (for example, primitive.go and constructed.go).

@JeyJeyGao JeyJeyGao mentioned this pull request Jan 2, 2024
Closed
4 tasks
@JeyJeyGao
test: add unit test
0ff9d3c 
Signed-off-by: Junjie Gao <[email protected]>
Two-Hearts
Two-Hearts approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JeyJeyGao JeyJeyGao closed this Jan 18, 2024
@Two-Hearts Two-Hearts mentioned this pull request Jan 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@priteshbandi priteshbandi priteshbandi left review comments

@Two-Hearts Two-Hearts Two-Hearts approved these changes

@gokarnm gokarnm Awaiting requested review from gokarnm gokarnm is a code owner

@justincormack justincormack Awaiting requested review from justincormack

@NiazFK NiazFK Awaiting requested review from NiazFK NiazFK is a code owner

@rgnote rgnote Awaiting requested review from rgnote rgnote is a code owner

@shizhMSFT shizhMSFT Awaiting requested review from shizhMSFT shizhMSFT is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JeyJeyGao @codecov-commenter @priteshbandi @shizhMSFT @Two-Hearts