Clean up DefaultServerTLSConfig & an unnecessary md5 #990

huitseeker · 2021-07-19T14:32:09Z

This impacts the check of the rootsnapshot file: CLI users would have to run the sha256sum command-line utility on it instead of md5sum

codecov-commenter · 2021-07-19T15:43:19Z

Codecov Report

Merging #990 (a1e2b5b) into master (ce3b61b) will decrease coverage by 0.00%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master     #990      +/-   ##
==========================================
- Coverage   54.81%   54.81%   -0.01%     
==========================================
  Files         277      277              
  Lines       18548    18548              
==========================================
- Hits        10168    10167       -1     
- Misses       7005     7006       +1     
  Partials     1375     1375

Flag	Coverage Δ
unittests	`54.81% <25.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/bootstrap/transit/cmd/pull.go	`11.32% <0.00%> (ø)`
engine/execution/state/delta/delta.go	`66.66% <ø> (ø)`
cmd/bootstrap/transit/cmd/utils.go	`30.57% <100.00%> (ø)`
cmd/util/ledger/migrations/storage_v4.go	`41.56% <0.00%> (-0.61%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce3b61b...a1e2b5b. Read the comment docs.

huitseeker · 2021-07-19T17:16:53Z

CC @m4ksio for the last commit a1e2b5b which is an SSA violation detected only by the golangci-lint updated in the present PR

huitseeker · 2021-07-19T18:30:20Z

Sister PR at onflow/flow#575 (update will be needed after this merges to update the console print at bootstrap)

tarakby

The changes look good to me 👌 but I let the team members involved in the operations side make sure there are no other impacts to these changes.

cmd/bootstrap/transit/cmd/crypto_test.go

zhangchiqing

Looks good

zhangchiqing · 2021-07-27T18:44:09Z

engine/execution/state/delta/delta.go

@@ -89,7 +89,7 @@ func (d Delta) MarshalJSON() ([]byte, error) {
 	return json.Marshal(m)
 }

-func (d Delta) UnmarshalJSON(data []byte) error {
+func (d *Delta) UnmarshalJSON(data []byte) error {


Why changing this?

as the commit message indicates, the assigment at line 106 occurs on an immediately-lost copy of the Delta otherwise.

Do not merge before onflow/flow-go#990 is merged. This PR updates our file-based user-visible hashes from MD5 to SHA256, a better, yet widely available hash function. The present PR does the same for the doc, at least in the format of the displayed hashes, rather than actual content: the doc is not updated at every Spork, and the output updated in this PR is here for demonstration only.

huitseeker · 2021-07-27T19:32:19Z

Dependent PR up at onflow/flow#585

vishalchangrani

lgtm

Followup of onflow#944: DefaultServerTLSConfig is unused, but will be in a further PR (onflow#989) Fix an insecurity, allowing for a TLS [MinVersion == 1.0](https://github.com/golang/go/blob/e3176bbc3ec7ab3889f02432f6fd088c90fc12dd/src/crypto/tls/common.go#L685) (CWE 295) Use gosec (through golang-ci) to prohibit that error.

The function was using an MD5 for checking, whereas a more secure option like SHA-256 would be preferred. See https://www.iacr.org/cryptodb/data/paper.php?pubkey=23903 (for example) for the (in-)security. SHA256 was chosen over Blake2 because there's a widely deployed CLI sha256sum on unix systems, and performance is not an issue here. Activates gosec G401 to fix such an issue, activates G501, G502, G503, G505 to tie up loose ends on less-secure hash functions..

This allows us to pick up golangci/golangci-lint#1930

The error SA4005: ineffective assignment to field Delta.Data was taken by taking a copy of the receiver

…cateFunc

huitseeker requested review from vishalchangrani, philipstanislaus and danuio July 19, 2021 14:32

huitseeker changed the title ~~Clean up an unnecessary md5~~ Clean up dead code & an unnecessary md5 Jul 19, 2021

huitseeker force-pushed the security1 branch 3 times, most recently from 3adeb8e to 04fa024 Compare July 19, 2021 14:46

huitseeker mentioned this pull request Jul 19, 2021

bringing up a GRPC secure sever for the access node #989

Merged

huitseeker force-pushed the security1 branch 3 times, most recently from 115c89c to c17b645 Compare July 19, 2021 15:37

huitseeker force-pushed the security1 branch 3 times, most recently from 5dff768 to c5ce172 Compare July 19, 2021 16:02

huitseeker requested review from m4ksio and ramtinms as code owners July 19, 2021 16:52

huitseeker force-pushed the security1 branch from 45cce2a to a1e2b5b Compare July 19, 2021 16:57

huitseeker mentioned this pull request Jul 19, 2021

[chore] Advise using sha256sum instead of md5sum onflow/flow#575

Merged

tarakby reviewed Jul 19, 2021

View reviewed changes

cmd/bootstrap/transit/cmd/crypto_test.go Outdated Show resolved Hide resolved

huitseeker force-pushed the security1 branch from a1e2b5b to e2a2331 Compare July 19, 2021 20:05

huitseeker changed the title ~~Clean up dead code & an unnecessary md5~~ Clean up DefaultServerTLSConfig & an unnecessary md5 Jul 19, 2021

huitseeker force-pushed the security1 branch from e2a2331 to 2767ead Compare July 20, 2021 10:44

huitseeker requested a review from zhangchiqing July 27, 2021 18:05

zhangchiqing approved these changes Jul 27, 2021

View reviewed changes

huitseeker force-pushed the security1 branch 2 times, most recently from b821192 to 01d5b47 Compare July 27, 2021 19:21

huitseeker mentioned this pull request Jul 27, 2021

Update MD5 -> SHA256 onflow/flow#585

Merged

huitseeker force-pushed the security1 branch 2 times, most recently from 9055135 to d66600f Compare July 27, 2021 19:42

vishalchangrani approved these changes Jul 27, 2021

View reviewed changes

huitseeker force-pushed the security1 branch 2 times, most recently from 780f8cf to b6bfbdd Compare July 27, 2021 20:20

huitseeker added 5 commits July 27, 2021 16:23

Update golangci-lint version

dd78258

This allows us to pick up golangci/golangci-lint#1930

Fix an ineffective assignment

915c2d5

The error SA4005: ineffective assignment to field Delta.Data was taken by taking a copy of the receiver

Suppress the G402 warning as that's been managed in verifyPeerCertifi…

680276f

…cateFunc

huitseeker force-pushed the security1 branch from b6bfbdd to 680276f Compare July 27, 2021 20:23

huitseeker merged commit a8a9e2b into onflow:master Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up DefaultServerTLSConfig & an unnecessary md5 #990

Clean up DefaultServerTLSConfig & an unnecessary md5 #990

huitseeker commented Jul 19, 2021

codecov-commenter commented Jul 19, 2021 •

edited

huitseeker commented Jul 19, 2021

huitseeker commented Jul 19, 2021

tarakby left a comment

zhangchiqing left a comment

zhangchiqing Jul 27, 2021

huitseeker Jul 27, 2021

huitseeker commented Jul 27, 2021

vishalchangrani left a comment

Clean up DefaultServerTLSConfig & an unnecessary md5 #990

Clean up DefaultServerTLSConfig & an unnecessary md5 #990

Conversation

huitseeker commented Jul 19, 2021

codecov-commenter commented Jul 19, 2021 • edited

Codecov Report

huitseeker commented Jul 19, 2021

huitseeker commented Jul 19, 2021

tarakby left a comment

Choose a reason for hiding this comment

zhangchiqing left a comment

Choose a reason for hiding this comment

zhangchiqing Jul 27, 2021

Choose a reason for hiding this comment

huitseeker Jul 27, 2021

Choose a reason for hiding this comment

huitseeker commented Jul 27, 2021

vishalchangrani left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 19, 2021 •

edited