feat: introduce act #4692

aranyia · 2024-05-22T10:04:44Z

Checklist

I have read the coding guide.
My change requires a documentation update, and I have done it.
I have added tests to cover my changes.
I have filled out the description and linked the related issues.

Description

Access control (ACT) implementation

The Access Control Trie (ACT) is a data structure that stores access control information for Swarm nodes. It is designed to provide access to personalised credentials permission to access a particular resource.

SWIP

See the ACT SWIP PR for more details.

See historical reviews and comments in the corresponding Solar Punk PR: Solar-Punk-Ltd#11

Open API Spec Version Changes (if applicable)

The version was not upgraded, but additions to the API spec can be review merged PR: Solar-Punk-Ltd#44

Motivation and Context (Optional)

See the ACT SWIP PR to understand the more motivation in more detail.

istae

commits must follow this guideline https://github.com/ethersphere/bee/blob/master/CODING.md#commit-messages

let's also make sure all cicd checks are passing

istae · 2024-05-27T15:08:07Z

pkg/kvs/kvs.go

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package kvs


let's move package as a subpackage to under either dynamicaccess or manifest

moved to accesscontrol

acha-bill

Mostly cosmetic changes

pkg/soc/testing/soc.go

pkg/kvs/kvs_test.go

pkg/dynamicaccess/session.go

pkg/dynamicaccess/accesslogic.go

pkg/dynamicaccess/accesslogic_test.go

pkg/dynamicaccess/history.go

pkg/dynamicaccess/grantee.go

pkg/dynamicaccess/grantee_test.go

pkg/dynamicaccess/controller.go

martinconic · 2024-06-01T18:34:17Z

pkg/api/chunk.go

@@ -139,6 +141,15 @@ func (s *Service) chunkUploadHandler(w http.ResponseWriter, r *http.Request) {
 		}
 	}

+	encryptedReference := chunk.Address()


Wouldn't be more clear to have chunkAddress instead encryptedReference ?

it should be reference really

renamed to reference

martinconic · 2024-06-03T17:37:02Z

pkg/api/soc.go

@@ -155,6 +157,15 @@ func (s *Service) socUploadHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	encryptedReference := sch.Address()


Maybe have schAddress instead of encryptedReference ?

renamed to reference

martinconic · 2024-06-03T17:37:39Z

pkg/api/soc.go

@@ -155,6 +157,15 @@ func (s *Service) socUploadHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	encryptedReference := sch.Address()
+	if headers.Act {
+		encryptedReference, err = s.actEncryptionHandler(r.Context(), w, putter, sch.Address(), headers.HistoryAddress)


Should we use the assigned variable instead of sch.Address()

call it reference and reuse it as argument of s.actEncryptionHandler

renamed to reference

martinconic · 2024-06-03T17:39:17Z

pkg/api/chunk.go

+			return
+		}
+	}
+
 	err = putter.Put(r.Context(), chunk)
 	if err != nil {
 		logger.Debug("chunk upload: write chunk failed", "chunk_address", chunk.Address(), "error", err)


Should we use the assigned variable instead of chunk.Address() ?

reference can change because of encryption so chunk.Adress() has to be used in the log

martinconic · 2024-06-03T17:48:05Z

pkg/dynamicaccess/accesslogic.go

+	if keys == nil {
+		return nil, nil, err
+	}
+	return keys[0], keys[1], err


What if len(keys) == 1 ?

martinconic · 2024-06-03T18:10:11Z

pkg/dynamicaccess/controller.go

+		granteesToAdd = gl.Get()
+	}
+
+	for _, grantee := range granteesToAdd {


Is there a way that maybe both addList and removeList might be nil or empty and here we are trying to iterate over a nil or empty list ?

and is that a problem?

Empty and nil slice has 0 length, iterating over it does nothing. I don't think it is a problem.

From the code perspective there should be no issues, just asking from the logic perspective.

zelig

pls rename dynamicaccess to just access or accesscontrol
accesslogic to access or keys

Add stardard compliant comment to each exported function.

zelig · 2024-06-04T20:24:29Z

pkg/dynamicaccess/accesslogic.go

+func (al *ActLogic) getKeys(publicKey *ecdsa.PublicKey) ([]byte, []byte, error) {
+	nonces := [][]byte{zeroByteArray, oneByteArray}
+	keys, err := al.Session.Key(publicKey, nonces)
+	if keys == nil {


Suggested change

if keys == nil {

if len(keys) != len(nonces) {

the length is the same as the length of nonces but indeed better check.

pkg/dynamicaccess/accesslogic.go

zelig · 2024-06-04T20:26:52Z

pkg/dynamicaccess/controller.go

+		granteesToAdd = gl.Get()
+	}
+
+	for _, grantee := range granteesToAdd {


and is that a problem?

zelig · 2024-06-04T20:27:50Z

pkg/dynamicaccess/controller.go

+	return granteeRef, egranteeRef, hRef, actRef, nil
+}
+
+func (c *ControllerStruct) Get(ctx context.Context, ls file.LoadSaver, publisher *ecdsa.PublicKey, encryptedglRef swarm.Address) ([]*ecdsa.PublicKey, error) {


Every exported function needs a stardard format comment, please let this sink in

Added comments to all exported functions and structs

pkg/dynamicaccess/controller.go

zelig · 2024-06-04T20:30:38Z

pkg/dynamicaccess/grantee.go

+	return swarm.NewAddress(refBytes), nil
+}
+
+var (


this to the top of the file

zelig · 2024-06-04T20:32:27Z

pkg/dynamicaccess/grantee.go

+}
+
+func deserializeBytes(data []byte) *ecdsa.PublicKey {
+	key, err := btcec.ParsePubKey(data)


do you really need another package an additional dependency here?
Use ecdsa serialisation functions

We need to use the btcec.S256 curve for deserialization, so btcec is a dependency here.

zelig · 2024-06-04T20:34:51Z

pkg/dynamicaccess/session_test.go

+	if !bytes.Equal(keys1[0], keys2[0]) {
+		t.Fatalf("shared secrets do not match %s, %s", hex.EncodeToString(keys1[0]), hex.EncodeToString(keys2[0]))
+	}
+	// if !bytes.Equal(keys1[1], keys2[1]) {


remove commented code

zelig · 2024-06-04T20:42:33Z

OpenAPI ci task failing on wrong credentials. Pls fix ASAP to make CICD green

Building some OpenAPI specs for you...
openapi/Swarm.yaml openapi/SwarmDebug.yaml

⚠️ This command is deprecated. Use "build" command instead.

Prerendering docs

🎉 bundled successfully in: /github/workspace/openapi/Swarm.yaml_fr0K4GtFkL2hD5nF33txjWz85u1HC6.html (1875 KiB) [⏱ 1.108s]
upload failed: openapi/Swarm.yaml_fr0K4GtFkL2hD5nF33txjWz85u1HC6.html to s3://swarm-openapi-specs/Swarm.yaml_fr0K4GtFkL2hD5nF33txjWz85u1HC6.html Unable to locate credentials

pkg/dynamicaccess/history.go

pkg/api/dynamicaccess.go

pkg/manifest/mantaray.go

pkg/dynamicaccess/session_test.go

pkg/dynamicaccess/accesslogic.go

pkg/dynamicaccess/accesslogic_test.go

aranyia · 2024-06-11T06:39:51Z

OpenAPI ci task failing on wrong credentials. Pls fix ASAP to make CICD green

Building some OpenAPI specs for you...
openapi/Swarm.yaml openapi/SwarmDebug.yaml

⚠️ This command is deprecated. Use "build" command instead.

Prerendering docs

🎉 bundled successfully in: /github/workspace/openapi/Swarm.yaml_fr0K4GtFkL2hD5nF33txjWz85u1HC6.html (1875 KiB) [⏱ 1.108s]
upload failed: openapi/Swarm.yaml_fr0K4GtFkL2hD5nF33txjWz85u1HC6.html to s3://swarm-openapi-specs/Swarm.yaml_fr0K4GtFkL2hD5nF33txjWz85u1HC6.html Unable to locate credentials

@istae I believe this problem originates from the CI configuration. There are some other pull requests with OpenApi doc changes which failing with the same issue. Could you confirm this?

istae · 2024-06-11T14:59:59Z

@aranyia you may ignore the openAPI failed check.

janos

Thanks for addressing my comments. LGTM

gacevicljubisa · 2024-07-23T14:04:24Z

If you need to test your changes against some other (specific) beekeeper branch, you can do it by changing the beekeeper branch on this line https://github.com/Solar-Punk-Ltd/bee/blob/3a7f51e25391504654f3b54ec458a9499fe523f9/.github/workflows/beekeeper.yml#L17 (replace with BEEKEEPER_BRANCH: "feat/act")
just make sure that you return it to "master" branch before the merge.

Also to use new "act" check defined in beekeeper in this PR ethersphere/beekeeper#408, in the same beekeeper.yaml (bee repoistory), if new "act" check should be used, in Integration tests new step should be added(something like this):

      - name: Test act
        id: act
        run: timeout ${TIMEOUT} beekeeper check --cluster-name local-dns --checks=ci-act

zelig · 2024-07-26T06:23:22Z

Makefile

 ifeq ($(BEEKEEPER_USE_SUDO), true)
 	sudo mv beekeeper_src/dist/beekeeper $(BEEKEEPER_INSTALL_DIR)
 else
 	mv beekeeper_src/dist/beekeeper $(BEEKEEPER_INSTALL_DIR)
 endif
 	rm -rf beekeeper_src
 endif
-	test -f ~/.beekeeper.yaml || curl -sSfL https://raw.githubusercontent.com/ethersphere/beekeeper/$(BEEKEEPER_BRANCH)/config/beekeeper-local.yaml -o ~/.beekeeper.yaml
-	mkdir -p ~/.beekeeper && curl -sSfL https://raw.githubusercontent.com/ethersphere/beekeeper/$(BEEKEEPER_BRANCH)/config/local.yaml -o ~/.beekeeper/local.yaml
+	test -f ~/.beekeeper.yaml || curl -sSfL https://raw.githubusercontent.com/Solar-Punk-Ltd/beekeeper/$(BEEKEEPER_BRANCH)/config/beekeeper-local.yaml -o ~/.beekeeper.yaml


i dont think this should be under SP

Reverted to ethersphere (beekeeper 0.17.3 contains ACT tests)

Co-authored-by: Ferenc Sárai <[email protected]>

bee-runner bot added the pull-request label May 22, 2024

aranyia marked this pull request as ready for review May 27, 2024 11:54

istae requested review from acha-bill and martinconic May 27, 2024 12:41

istae reviewed May 27, 2024

View reviewed changes

istae requested a review from janos May 30, 2024 14:26

acha-bill requested changes Jun 3, 2024

View reviewed changes

martinconic reviewed Jun 3, 2024

View reviewed changes

zelig requested changes Jun 4, 2024

View reviewed changes

janos requested changes Jun 6, 2024

View reviewed changes

aranyia requested review from acha-bill, janos and zelig July 3, 2024 13:28

janos approved these changes Jul 5, 2024

View reviewed changes

martinconic approved these changes Jul 9, 2024

View reviewed changes

aranyia force-pushed the act branch 2 times, most recently from 8f6be5b to 20e42fa Compare July 9, 2024 14:19

istae approved these changes Jul 17, 2024

View reviewed changes

acha-bill approved these changes Jul 19, 2024

View reviewed changes

aranyia force-pushed the act branch 2 times, most recently from 5cb87b6 to cb80041 Compare July 25, 2024 09:39

zelig approved these changes Jul 26, 2024

View reviewed changes

Ferenc Sárai and others added 3 commits August 2, 2024 15:27

feat: access logic trie

e1411ed

refactor(act): fix pr comments (#50)

35a3353

ci: disable commit linting (temporarily) (#63)

722d096

bosi95 and others added 7 commits August 2, 2024 15:27

refactor(act): error handling and testing (#64)

caaa077

ci: enable commit linting

d4b59a8

fix: kvsmock race condition (#68)

82f4520

Co-authored-by: Ferenc Sárai <[email protected]>

ci: use beekeeper feat/act branch, add ci-act test

655e0e2

ci: fix typo

d737912

ci: fix beekeper repository URl

fbad45c

ci: add test for act failure

5029b2f

aranyia force-pushed the act branch from cb80041 to 5029b2f Compare August 2, 2024 11:27

Ferenc Sárai and others added 2 commits August 5, 2024 08:36

ci: revert beekeeper branch to master, repository to ethersphere

d385c50

ci: add act test to beekeeper (#69)

5bebe88

Co-authored-by: Ferenc Sárai <[email protected]>

istae merged commit 7e45f3e into ethersphere:master Aug 7, 2024
13 of 14 checks passed

aranyia mentioned this pull request Aug 8, 2024

ACT Solar-Punk-Ltd/bee#11

Closed

4 tasks

feat: introduce act #4692

feat: introduce act #4692

Conversation

aranyia commented May 22, 2024 • edited by zelig Loading

Checklist

Description

Access control (ACT) implementation

SWIP

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

istae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acha-bill 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

martinconic Jun 3, 2024 • edited Loading

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

bosi95 Jun 6, 2024 • edited Loading

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

zelig 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig commented Jun 4, 2024

aranyia commented Jun 11, 2024

istae commented Jun 11, 2024

janos left a comment

Choose a reason for hiding this comment

gacevicljubisa commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aranyia commented May 22, 2024 •

edited by zelig

Loading

martinconic Jun 3, 2024 •

edited

Loading

bosi95 Jun 6, 2024 •

edited

Loading

gacevicljubisa commented Jul 23, 2024 •

edited

Loading