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.

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

ipfs: add support to pull images by ref #1968

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wswsmao
Copy link
Contributor

@wswsmao wswsmao commented Feb 8, 2025

The solution for issue #1944 has been finalized, with the main design concepts outlined as follows:

  1. IPNS
    IPNS provides the ipfs name publish and ipfs name resolve commands, which allow for associating a file with an IPNS name, enabling publishing and retrieval.
    For IPFS images, we can consider publishing the root descriptor file, allowing other nodes to resolve this file to pull the image. IPNS requires key management to control the publishing and resolution of IPNS names, with each key corresponding to a unique IPNS name.
    This characteristic is well-suited for associating with image references, ultimately establishing the following relationship:
    Image ref -> Key -> Root Descriptor -> Image File

  2. Deterministic Keys
    Different nodes will share the same deterministic key algorithm to ensure that each node receives the same key for the same image name. This allows every node to pull and push images. Specifically:

    • Pull Phase​: The corresponding image ref can be resolved using just the public key ID.
    • Push Phase​: The private key must first be imported into the local IPFS. After uploading the image, the updated root descriptor file will be published.

Additional Notes
This submission is compatible with the existing method. During the push / pull phase, the input will be checked to determine if it is a ref. If users still prefer to use CID, this method will remain valid.

Feature Demonstration
On Node A:

./ctr-remote i ipfs-push --estargz=false docker.io/abushwang/oc9-busybox:org
or
./ctr-remote i ipfs-push --estargz=true docker.io/abushwang/oc9-busybox:esgz

On other nodes:

./ctr-remote i rpull --snapshotter=overlayfs --ipfs docker.io/abushwang/oc9-busybox:org
or
./ctr-remote i rpull --snapshotter=stargz --ipfs docker.io/abushwang/oc9-busybox:esgz

Other nodes can also rebuild docker.io/abushwang/oc9-busybox:org, and then run:

./ctr-remote i ipfs-push --estargz=false docker.io/abushwang/oc9-busybox:org

At this point, Node A can pull the updated image:

./ctr-remote i rpull --snapshotter=overlayfs --ipfs docker.io/abushwang/oc9-busybox:org

@wswsmao wswsmao force-pushed the ipfs branch 7 times, most recently from 20d3c42 to bfee105 Compare February 8, 2025 08:00
cmd/go.mod Outdated
@@ -49,6 +49,7 @@ require (
github.com/containernetworking/plugins v1.5.1 // indirect
github.com/cpuguy83/go-md2man/v2 v2.0.5 // indirect
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
github.com/decred/dcrd/dcrec/secp256k1/v4 v4.2.0 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this dependency on "Decred" cryptocurrency to avoid potential confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a bit difficult. 'Decred' is introduced by the following two packages:

github.com/libp2p/go-libp2p/core/crypto
github.com/libp2p/go-libp2p/core/peer

These two packages are used to compute the public key and peer ID in IPFS.
The Kubo project also introduces 'Decred':
https://github.com/ipfs/kubo/blob/master/go.mod#L119

Copy link
Member

Choose a reason for hiding this comment

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

Can we just exec the ipfs command for computing them?

Copy link
Member

Choose a reason for hiding this comment

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

Actually we got a compliant about a dependency on a bitcoin-originated library in the past, although it was not for mining nor trading bitcoin

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me think. Do you mean that in ipnskey.go's (g *DetKeyGen) generateKey(name string), after we obtain the seed bytes for the deterministic keys through reader.Read(seedBytes), we execute the IPFS command using an OS command (for example, exec.Command) to calculate the public key and peer ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, actually, there's no need to use the IPFS command; we can just use the API. We can import the private key into IPFS and then use the list API to obtain the public key ID and peer ID. I'll update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wswsmao wswsmao force-pushed the ipfs branch 2 times, most recently from 6b511ab to 3545af1 Compare February 12, 2025 07:01
cmd/go.mod Outdated
@@ -49,6 +49,7 @@ require (
github.com/containernetworking/plugins v1.5.1 // indirect
github.com/cpuguy83/go-md2man/v2 v2.0.5 // indirect
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
github.com/decred/dcrd/dcrec/secp256k1/v4 v4.2.0 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.

Copy link
Contributor Author

@wswsmao wswsmao Feb 13, 2025

Choose a reason for hiding this comment

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

done
Refer to: #1968 (comment)


// KeyStore manages a collection of DetKeyGen instances
type KeyStore struct {
store map[string]*DetKeyGen
Copy link
Member

Choose a reason for hiding this comment

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

Is this tolerant to restart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KeyStore has been removed now

Comment on lines 277 to 287
if err := json.NewDecoder(resp.Body).Decode(&rs); err != nil {
return "", err
}

parts := strings.Split(rs.Path, "/")
if len(parts) < 3 || parts[1] != "ipfs" {
return "", fmt.Errorf("invalid resolved path format: %s", rs.Path)
}

return parts[2], nil
Copy link
Member

Choose a reason for hiding this comment

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

needs docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, add doc to docs/ipfs.md

Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment of link to ipfs api docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 17 to 29
package ipnskey

import (
"bytes"
"crypto/ed25519"
"crypto/sha256"
"crypto/x509"
"encoding/pem"
"fmt"
"sync"

"github.com/libp2p/go-libp2p/core/crypto"
"github.com/libp2p/go-libp2p/core/peer"
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be imported to stargz snapshotter but should be done in ipfs daemon.

Copy link
Contributor Author

@wswsmao wswsmao Feb 13, 2025

Choose a reason for hiding this comment

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

done
Refer to: #1968 (comment)

@wswsmao wswsmao force-pushed the ipfs branch 3 times, most recently from b7a87c0 to 6c36913 Compare February 13, 2025 11:46
@wswsmao
Copy link
Contributor Author

wswsmao commented Feb 13, 2025

@AkihiroSuda @ktock
The PR has been updated with the following changes:

  • Removed unnecessary dependencies: The generated key is now imported into IPFS, and the peer ID can be retrieved later using IPFS's list API, thus avoiding redundant dependencies.
  • Since the peer ID and related information are stored in IPFS, there is no longer a need for the KeyStore hash table to record the ref and peer ID; this can be accomplished directly through the list API.
  • Updated the docs/ipfs.md documentation.

Copy link
Member

@ktock ktock left a comment

Choose a reason for hiding this comment

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

Can you add integration tests?

cmd/go.mod Outdated
github.com/ipfs/go-cid v0.1.0 // indirect
github.com/ipfs/go-cid v0.4.1 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

@wswsmao wswsmao Feb 14, 2025

Choose a reason for hiding this comment

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

no need update to v0.4.1
done

Comment on lines 277 to 287
if err := json.NewDecoder(resp.Body).Decode(&rs); err != nil {
return "", err
}

parts := strings.Split(rs.Path, "/")
if len(parts) < 3 || parts[1] != "ipfs" {
return "", fmt.Errorf("invalid resolved path format: %s", rs.Path)
}

return parts[2], nil
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment of link to ipfs api docs?

limitations under the License.
*/

package ipnskey
Copy link
Member

Choose a reason for hiding this comment

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

needs explanation comments and link to docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wswsmao wswsmao force-pushed the ipfs branch 4 times, most recently from 1eb20d7 to a015bb6 Compare February 14, 2025 08:29
@wswsmao
Copy link
Contributor Author

wswsmao commented Feb 14, 2025

Hi, @ktock
The PR has been updated with the following changes:

  • Add integration tests in script/integration/containerd/entrypoint.sh
  • Add comments of link to ipfs api docs for package ipnskey, Resolve, Publish and importKey

@wswsmao wswsmao requested a review from ktock February 19, 2025 11:16
client = http.DefaultClient
}

ipfsAPINamePublish := c.Address + "/api/v0/name/publish"
Copy link
Member

Choose a reason for hiding this comment

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

Can we have docs about lifetime and republishing of contents?

Copy link
Contributor Author

@wswsmao wswsmao Feb 20, 2025

Choose a reason for hiding this comment

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

limitations under the License.
*/

// Package ipnskey provides functionality for generating deterministic Ed25519 key pairs
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@ktock ktock Feb 20, 2025

Choose a reason for hiding this comment

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

@wswsmao can you check this ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I got a little trouble, check done
image

Copy link
Member

Choose a reason for hiding this comment

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

So this sounds like all users on the IPFS network can get the private key of an image ref and update and sign the image. How can a user verify the image is updated by a trusted one, just like the default IPNS settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using IPFS, we have configured a IPFS private network by default, where the nodes within this private network are considered trusted. Additionally, nodes outside of the private network, even if they obtain the private key, will not be able to publish or resolve image files.

@wswsmao
Copy link
Contributor Author

wswsmao commented Feb 21, 2025

all review done @ktock

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants