chore: implement signing of extensions #79

Emyrk · 2024-12-11T22:49:52Z

What this does

Signs extensions so that this warning goes away. Will close #65

How it does this.

On add, the plaintext payload to sign is generated.
Use --sign to have the marketplace sign extensions, without this flag, the marketplace acts as it used to.

Manifest includes signed extension dynamically:

Lines 70 to 74 in cc33da1

    
           manifest.Assets.Asset = append(manifest.Assets.Asset, VSIXAsset{ 
        
           	Type:        VSIXSignatureType, 
        
           	Path:        sigzipFilename, 
        
           	Addressable: "true", 
        
           })

Payload is signed dynamically on demand:

code-marketplace/storage/signature.go

Lines 80 to 106 in cc33da1

    
           if s.SigningEnabled() && filepath.Base(fp) == "p7s.sig" { 
        
           	// This file must exist, and it is always empty 
        
           	return mem.NewFileHandle(mem.CreateFile("p7s.sig")), nil 
        
           } 
        
           if s.SigningEnabled() && filepath.Base(fp) == sigzipFilename { 
        
           	// hijack this request, sign the sig manifest 
        
           	manifest, err := s.Storage.Open(ctx, filepath.Join(filepath.Dir(fp), sigManifestName)) 
        
           	if err != nil { 
        
           		fmt.Println(err) 
        
           		return nil, xerrors.Errorf("open signature manifest: %w", err) 
        
           	} 
        
           	defer manifest.Close() 
        
           	manifestData, err := io.ReadAll(manifest) 
        
           	if err != nil { 
        
           		return nil, xerrors.Errorf("read signature manifest: %w", err) 
        
           	} 
        
           	signed, err := extensionsign.SignAndZipManifest(s.Signer, manifestData) 
        
           	if err != nil { 
        
           		return nil, xerrors.Errorf("sign and zip manifest: %w", err) 
        
           	} 
        
           	f := mem.NewFileHandle(mem.CreateFile(sigzipFilename)) 
        
           	_, err = f.Write(signed) 
        
           	return f, err 
        
           }

This means no signed values ever hit the storage.

Work still needed

This implementation is MVP. It still needs to handle importing the signing key, and handling existing extensions. As the signature payload is generated on add. All of this will be handled in a follow up PR.

Refactors

FileServer used to return a http.Handler. I changed this to Open(ctx context.Context, name string) (fs.File, error). This makes the storage closer to a fs.FS, meaning I could implement Signatures as a wrapper. Rather than having to mutate stores.
Command server flags are defined in 1 place, and used throughout.

Unfortunate

This change requires the file to be fully read from network before serving from Artifactory. Do we think keeping these bytes in memory is an issue? Rather than streaming?
- Keep in mind for signatures to work, the payload needs to be signed, so it has to enter memory. Just unfortunate it always needs to, even when not using signatures.
- This is because I am using afero for mem files, and do not see a way to stream.

Emyrk · 2024-12-12T19:56:01Z

storage/artifactory.go

+	for _, file := range extra {
+		// TODO: I think this is correct?
+		_, err := s.upload(ctx, path.Join(dir, file.RelativePath), bytes.NewReader(file.Content))
+		if err != nil {
+			return "", err
+		}
+	}
+


I think this is right?

That looks right to me!

code-asher

No blocking comments, it looks great! I have not had a chance to run this with code-server or anything yet, and I definitely will, but no need to block on me doing that I think.

code-asher · 2024-12-12T21:54:21Z

cli/add.go

-		repo        string
-	)
-
+	addFlags, opts := serverFlags()


Oo yeah very nice

cli/server.go

code-asher · 2024-12-12T22:02:23Z

cli/server.go

+		cmd.Flags().StringVar(&opts.Artifactory, "artifactory", "", "Artifactory server URL.")
+		cmd.Flags().StringVar(&opts.Repo, "repo", "", "Artifactory repository.")
+		cmd.Flags().DurationVar(&opts.ListCacheDuration, "list-cache-duration", time.Minute, "The duration of the extension cache.")
+		cmd.Flags().BoolVar(&sign, "sign", false, "Sign extensions.")


Sort of a similar thought process here, I know it is hidden at the moment, but remove with --sign might not make sense, I think? Also not sure about add, still need to go through the rest of the PR, but we are signing on demand right? So only with server? I am assuming we just always create the signature manifest on add.

Mostly thinking of it in terms of what a user might see and think when they run subcommand --help.

You are right it does not make the most sense, but it is hidden. The sign flag will be removed though, replaced by something that will parse a key or cert.

See the flags in my other PR:

code-marketplace/cli/server.go

Lines 35 to 37 in 449f77c

cmd.Flags().StringArrayVar(&certificates, "certs", []string{}, "The path to certificates that match the signing key.")

cmd.Flags().StringVar(&signingKeyFile, "key", "", "The path to signing key file in PEM format.")

cmd.Flags().BoolVar(&opts.SaveSigZips, "save-sigs", false, "Save signed extensions to disk for debugging.")

I do have a flag to save the signature to disk. Makes it easier to debug

I'm going to leave sign for now. I intend to fix it before being unhidden, anyone using it will be broken when I change things.

When I fix it, I'll make the flags make sense.

code-asher · 2024-12-12T22:36:45Z

storage/artifactory.go

+	for _, file := range extra {
+		// TODO: I think this is correct?
+		_, err := s.upload(ctx, path.Join(dir, file.RelativePath), bytes.NewReader(file.Content))
+		if err != nil {
+			return "", err
+		}
+	}
+


That looks right to me!

storage/artifactory.go

storage/local.go

code-asher · 2024-12-12T22:59:38Z

storage/artifactory.go

-	})
+	}
+
+	f := mem.NewFileHandle(mem.CreateFile(fp))


Possibly a stupid question, and I have not seen the rest of the usage yet, but would it make sense to return the reader to avoid having to hold the files in memory? Then the caller can decide to read it in if necessary (for signing, which then returns a reader for the sigzip after doing its thing) or just stream it as usual for any other file.

Edit: nvm, I see how this makes it easier because it mirrors the local one where we can just use http.Dir().Open() which is nice.

Or...is it possible to implement a File that Reads from the HTTP response? Not sure how Stat would be implemented though. In any case, probably not a big deal. The biggest file is probably the .vsix, and surely there are not multi-gigabyte .vsix files I would hope.

Although, could it maybe be an issue for large deployments serving many files to many people at once?

I was really wondering about this too, but ended up not spending the time to try to fix it.

Or...is it possible to implement a File that Reads from the HTTP response? Not sure how Stat would be implemented though. In any case, probably not a big deal. The biggest file is probably the .vsix, and surely there are not multi-gigabyte .vsix files I would hope.

This would be ideal, you can implement Stat rather easily, it's implementing the io.Seeker that was problematic when I spent a few minutes on it.

I think we should revisit this and try to fix it. I'll throw on a comment. If we re-implement http.FileServer, we might be able to make our own easier to implement interface too. Ignore things like Stat

Awesome, that sounds good to me.

storage/signature.go

storage/storage_test.go

storage/artifactory.go

Emyrk · 2024-12-13T15:38:09Z

chore: more work towards supporting p7s #81
chore: implement signing of extensions #79 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Emyrk changed the title ~~chore: begin work implementing vsix signatures, manifest matching~~ chore: implement signing of extensions Dec 12, 2024

Emyrk added 8 commits December 12, 2024 13:11

chore: begin work implementing vsix signatures, manifest matching

644c355

properly zip sigzips

fe51069

have some sort of signing going

3b91874

chore: signatures working, time to refactor

75208fe

chore: refactor cli to support resuing the server flags

f4091e7

move extensionsign pkg

2e4c55e

chore: implement signature storage, still need to load a key

a72ead1

user afero mem files

21661f0

Emyrk force-pushed the stevenmasley/vsix_signatures branch from 166a21d to 21661f0 Compare December 12, 2024 19:11

Emyrk added 5 commits December 12, 2024 13:15

bump go version

62d0a17

fix artifactory mem files

404d3a7

setup signer opts pass through

0cdd5a3

fixup tests

cc33da1

Add more comments

43d8b50

Emyrk marked this pull request as ready for review December 12, 2024 19:45

add comments

77a862c

Emyrk commented Dec 12, 2024

View reviewed changes

Emyrk added 4 commits December 12, 2024 14:01

update golangci lint in gh actions

f8b3714

handle cached artifactory manifests

35c997b

fix err groups

4f7cd91

fix err groups

1ab2dcc

Emyrk requested a review from code-asher December 12, 2024 20:55

code-asher approved these changes Dec 12, 2024

View reviewed changes

Emyrk added 2 commits December 13, 2024 09:29

PR fixes

e51ea05

server flags only on server

dd76177

Emyrk mentioned this pull request Dec 13, 2024

chore: more work towards supporting p7s #81

Draft

Emyrk added 2 commits December 13, 2024 09:43

add a comment to prevent copy into memory

adb3c66

fix artifactory read

0fa5b4e

Emyrk merged commit f35db41 into main Dec 13, 2024
6 checks passed

Emyrk deleted the stevenmasley/vsix_signatures branch December 13, 2024 16:52

github-actions bot locked and limited conversation to collaborators Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: implement signing of extensions #79

chore: implement signing of extensions #79

Emyrk commented Dec 11, 2024 •

edited

Loading

Emyrk Dec 12, 2024

code-asher Dec 12, 2024

code-asher left a comment

code-asher Dec 12, 2024

code-asher Dec 12, 2024

Emyrk Dec 13, 2024

Emyrk Dec 13, 2024

code-asher Dec 13, 2024

code-asher Dec 12, 2024

code-asher Dec 12, 2024

Emyrk Dec 13, 2024

code-asher Dec 13, 2024

Emyrk commented Dec 13, 2024 •

edited

Loading

	manifest.Assets.Asset = append(manifest.Assets.Asset, VSIXAsset{
	Type: VSIXSignatureType,
	Path: sigzipFilename,
	Addressable: "true",
	})

	if s.SigningEnabled() && filepath.Base(fp) == "p7s.sig" {
	// This file must exist, and it is always empty
	return mem.NewFileHandle(mem.CreateFile("p7s.sig")), nil
	}
	if s.SigningEnabled() && filepath.Base(fp) == sigzipFilename {
	// hijack this request, sign the sig manifest
	manifest, err := s.Storage.Open(ctx, filepath.Join(filepath.Dir(fp), sigManifestName))
	if err != nil {
	fmt.Println(err)
	return nil, xerrors.Errorf("open signature manifest: %w", err)
	}
	defer manifest.Close()

	manifestData, err := io.ReadAll(manifest)
	if err != nil {
	return nil, xerrors.Errorf("read signature manifest: %w", err)
	}

	signed, err := extensionsign.SignAndZipManifest(s.Signer, manifestData)
	if err != nil {
	return nil, xerrors.Errorf("sign and zip manifest: %w", err)
	}

	f := mem.NewFileHandle(mem.CreateFile(sigzipFilename))
	_, err = f.Write(signed)
	return f, err
	}

	cmd.Flags().StringArrayVar(&certificates, "certs", []string{}, "The path to certificates that match the signing key.")
	cmd.Flags().StringVar(&signingKeyFile, "key", "", "The path to signing key file in PEM format.")
	cmd.Flags().BoolVar(&opts.SaveSigZips, "save-sigs", false, "Save signed extensions to disk for debugging.")

chore: implement signing of extensions #79

chore: implement signing of extensions #79

Conversation

Emyrk commented Dec 11, 2024 • edited Loading

What this does

Work still needed

Refactors

Unfortunate

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-asher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk commented Dec 13, 2024 • edited Loading

Emyrk commented Dec 11, 2024 •

edited

Loading

Emyrk commented Dec 13, 2024 •

edited

Loading