tools: verify with gpg if md5 is not present in update-icu #50507

islandryu · 2023-11-01T15:16:42Z

Fix: #50498

The problem may be that md5 is not present in the icu, but even in such a case, I used .asc to pass the validation.

If the absence of md5 is clearly an icu issue, this PR will be closed.

nodejs-github-bot · 2023-11-01T15:16:47Z

Review requested:

@nodejs/security-wg

islandryu · 2023-11-01T15:38:55Z

tools/dep_updaters/update-icu.sh

@@ -41,6 +41,9 @@ NEW_VERSION_TGZ="icu4c-${LOW_DASHED_NEW_VERSION}-src.tgz"
 NEW_VERSION_TGZ_URL="https://github.com/unicode-org/icu/releases/download/release-${DASHED_NEW_VERSION}/$NEW_VERSION_TGZ"

 NEW_VERSION_MD5="https://github.com/unicode-org/icu/releases/download/release-${DASHED_NEW_VERSION}/icu4c-${LOW_DASHED_NEW_VERSION}-src.md5"
+NEW_VERSION_TGZ_ASC_URL="https://github.com/unicode-org/icu/releases/download/release-${DASHED_NEW_VERSION}/icu4c-${LOW_DASHED_NEW_VERSION}-src.tgz.asc"
+
+KEY_URL="https://raw.githubusercontent.com/unicode-org/icu/release-$(echo $NEW_VERSION | sed 's/\./-/')/KEYS"


I don't know myself if the key is correct at this URL.

targos · 2023-11-01T15:59:32Z

We need to decide what to do with https://github.com/nodejs/node/blob/main/tools/icu/current_ver.dep if we can't use md5 anymore (I don't know what's the purpose of that file)

richardlau · 2023-11-01T16:20:40Z

We need to decide what to do with https://github.com/nodejs/node/blob/main/tools/icu/current_ver.dep if we can't use md5 anymore (I don't know what's the purpose of that file)

It's used to validate ICU downloads if configure is run with --with-icu-source with a URL.

node/BUILDING.md

Lines 778 to 782 in a037b88

    
           From a tarball URL: 
        
           ```bash 
        
           ./configure --with-intl=full-icu --with-icu-source=http://url/to/icu.tgz 
        
           ```

node/configure.py

Lines 1646 to 1679 in a037b88

    
           def configure_intl(o): 
        
             def icu_download(path): 
        
               depFile = tools_path / 'icu' / 'current_ver.dep' 
        
               icus = json.loads(depFile.read_text(encoding='utf-8')) 
        
               # download ICU, if needed 
        
               if not os.access(options.download_path, os.W_OK): 
        
                 error('''Cannot write to desired download path. 
        
                   Either create it or verify permissions.''') 
        
               attemptdownload = nodedownload.candownload(auto_downloads, "icu") 
        
               for icu in icus: 
        
                 url = icu['url'] 
        
                 (expectHash, hashAlgo, allAlgos) = nodedownload.findHash(icu) 
        
                 if not expectHash: 
        
                   error(f'''Could not find a hash to verify ICU download. 
        
                     {depFile} may be incorrect. 
        
                     For the entry {url}, 
        
                     Expected one of these keys: {' '.join(allAlgos)}''') 
        
                 local = url.split('/')[-1] 
        
                 targetfile = Path(options.download_path, local) 
        
                 if not targetfile.is_file(): 
        
                   if attemptdownload: 
        
                     nodedownload.retrievefile(url, targetfile) 
        
                 else: 
        
                   print(f'Re-using existing {targetfile}') 
        
                 if targetfile.is_file(): 
        
                   print(f'Checking file integrity with {hashAlgo}:\r') 
        
                   gotHash = nodedownload.checkHash(targetfile, hashAlgo) 
        
                   print(f'{hashAlgo}:      {gotHash}  {targetfile}') 
        
                   if expectHash == gotHash: 
        
                     return targetfile 
        
                   warn(f'Expected: {expectHash}      *MISMATCH*') 
        
                   warn(f'\n ** Corrupted ZIP? Delete {targetfile} to retry download.\n') 
        
               return None

islandryu · 2023-11-02T04:26:44Z

It's used to validate ICU downloads if configure is run with --with-icu-source with a URL.

If we're going to verify it based on .asc, we're going to need a dedicated process at the point you indicated.

I think it would be good to save the public key and signature information in current_ver.dep or a separate file and verify it as well as md5.

islandryu · 2023-11-05T01:38:48Z

tools/configure.d/nodedownload.py

+def checkGPG(targetfile, key_url, sig_url):
+    key_data = download_file(key_url)


gpg verification of modification based on url stored in current_ver.dep

bnoordhuis · 2023-11-05T10:06:04Z

cc @srl295 in case you missed this pull request

srl295 · 2023-11-06T15:48:20Z

cc @srl295 in case you missed this pull request

I missed it, but it's fixed upstream

srl295

makes sense

islandryu · 2023-11-07T11:42:35Z

tools/dep_updaters/update-icu.sh

@@ -84,6 +82,7 @@ rm -rf "$DEPS_DIR/icu"

 perl -i -pe "s|\"url\": .*|\"url\": \"$NEW_VERSION_TGZ_URL\",|" "$TOOLS_DIR/icu/current_ver.dep"

+perl -i -pe "$REGEX" "$TOOLS_DIR/icu/current_ver.dep"


conflict resolved
and changed writing of current_ver.dep to be done after icu file change

aduh95 · 2024-05-11T18:25:17Z

This needs a rebase.

ICU releases may not include md5 files to verify code Added a branch to verify from .asc file using gpg in such cases Fixes: nodejs#50498

islandryu · 2024-05-20T11:15:36Z

The rebase is done.

srl295

If gpg isn't installed AND the md5 is present, then the md5 should be used.

https://unicode-org.atlassian.net/jira/software/c/projects/ICU/issues/ICU-22567 was a bug and not a new direction for ICU.

I would suggest a logic change here:

if the md5 signature is available, verify it.
if gpg is installed AND the signature is available, verify it.

Warn if neither are available.
Warn if gpg wasn't installed (that is, could not verify provenance.)

srl295 · 2024-05-20T16:48:07Z

configure.py

-
-        warn(f'Expected: {expectHash}      *MISMATCH*')
-        warn(f'\n ** Corrupted ZIP? Delete {targetfile} to retry download.\n')
+        if "gpg" in icu:


Should this also verify that gpg is present and installed?

@srl295

Changed to check both gpg and md5.

Only warn when md5 cannot be obtained.

Only warn if gpg signature cannot be obtained or gpg is not installed

If neither is available, warn and no update.

If either fails verification, warn out and no update

srl295 · 2024-05-20T16:54:20Z

Absence of md5 is an ICU issue and was fixed, However verifying with gpg is not a bad idea.

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 1, 2023

islandryu marked this pull request as draft November 1, 2023 15:17

islandryu changed the title ~~Verify with gpg if md5 is not present in update-icu~~ tools: verify with gpg if md5 is not present in update-icu Nov 1, 2023

islandryu force-pushed the chore/enableVerifyWithSignatureIcu branch from 7eda2d2 to 996127a Compare November 1, 2023 15:35

islandryu commented Nov 1, 2023

View reviewed changes

islandryu marked this pull request as ready for review November 1, 2023 15:40

islandryu force-pushed the chore/enableVerifyWithSignatureIcu branch from 996127a to 582e809 Compare November 5, 2023 01:32

islandryu commented Nov 5, 2023

View reviewed changes

srl295 approved these changes Nov 6, 2023

View reviewed changes

islandryu force-pushed the chore/enableVerifyWithSignatureIcu branch from 582e809 to a05004f Compare November 7, 2023 11:30

islandryu commented Nov 7, 2023

View reviewed changes

islandryu added 3 commits May 20, 2024 20:12

tools: verify with gpg if md5 is not present in update-icu

a95ea15

ICU releases may not include md5 files to verify code Added a branch to verify from .asc file using gpg in such cases Fixes: nodejs#50498

tools: add gpg verification using current_ver.dep

79197af

tools: Write current_ver.dep after file change

368605a

islandryu force-pushed the chore/enableVerifyWithSignatureIcu branch from a05004f to 368605a Compare May 20, 2024 11:12

aduh95 requested a review from srl295 May 20, 2024 15:10

srl295 requested changes May 20, 2024

View reviewed changes

islandryu added 2 commits May 21, 2024 20:18

tools: Check icu in both md5 and gpg

c5927dd

refactor:Change the order of branches

33d90e0

islandryu requested a review from srl295 May 21, 2024 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: verify with gpg if md5 is not present in update-icu #50507

tools: verify with gpg if md5 is not present in update-icu #50507

islandryu commented Nov 1, 2023

nodejs-github-bot commented Nov 1, 2023

islandryu Nov 1, 2023

targos commented Nov 1, 2023

richardlau commented Nov 1, 2023

islandryu commented Nov 2, 2023

islandryu Nov 5, 2023

bnoordhuis commented Nov 5, 2023

srl295 commented Nov 6, 2023

srl295 left a comment

islandryu Nov 7, 2023

aduh95 commented May 11, 2024

islandryu commented May 20, 2024

srl295 left a comment

srl295 May 20, 2024

islandryu May 21, 2024

srl295 commented May 20, 2024

		def checkGPG(targetfile, key_url, sig_url):
		key_data = download_file(key_url)

		@@ -84,6 +82,7 @@ rm -rf "$DEPS_DIR/icu"

		perl -i -pe "s\|\"url\": .*\|\"url\": \"$NEW_VERSION_TGZ_URL\",\|" "$TOOLS_DIR/icu/current_ver.dep"

		perl -i -pe "$REGEX" "$TOOLS_DIR/icu/current_ver.dep"

tools: verify with gpg if md5 is not present in update-icu #50507

Are you sure you want to change the base?

tools: verify with gpg if md5 is not present in update-icu #50507

Conversation

islandryu commented Nov 1, 2023

nodejs-github-bot commented Nov 1, 2023

islandryu Nov 1, 2023

Choose a reason for hiding this comment

targos commented Nov 1, 2023

richardlau commented Nov 1, 2023

islandryu commented Nov 2, 2023

islandryu Nov 5, 2023

Choose a reason for hiding this comment

bnoordhuis commented Nov 5, 2023

srl295 commented Nov 6, 2023

srl295 left a comment

Choose a reason for hiding this comment

islandryu Nov 7, 2023

Choose a reason for hiding this comment

aduh95 commented May 11, 2024

islandryu commented May 20, 2024

srl295 left a comment

Choose a reason for hiding this comment

srl295 May 20, 2024

Choose a reason for hiding this comment

islandryu May 21, 2024

Choose a reason for hiding this comment

srl295 commented May 20, 2024