TrueCloudLab/certificates
15
0
Fork 2
Code Issues 1 Pull requests 1 Releases Activity Actions

Add NNS Challenge support #1

Merged
alexvanin merged 1 commit from mbiryukova/certificates:feature/add_nns_challenge into tcl/master 2023-07-31 11:24:31 +00:00
Conversation 2 Commits 1 Files changed 10 +346 -8
mbiryukova commented 2023-07-26 07:48:50 +00:00
Member
Copy link
No description provided.
mbiryukova requested review from alexvanin 2023-07-26 07:50:08 +00:00
mbiryukova requested review from pogpp 2023-07-26 07:50:08 +00:00
pogpp reviewed 2023-07-26 08:27:58 +00:00
acme/api/order.go Outdated
@ -393,3 +393,3 @@
chTypes = []acme.ChallengeType{acme.HTTP01, acme.TLSALPN01}
case acme.DNS:
chTypes = []acme.ChallengeType{acme.DNS01}
chTypes = []acme.ChallengeType{acme.DNS01, acme.NNS01}
pogpp commented 2023-07-26 08:27:58 +00:00
Member
Copy link

why can't we add separate case
case acme.NNS:

cause we can add this type in acme/order.go

why can't we add separate case `case acme.NNS:` cause we can add this type in acme/order.go
👍 1
mbiryukova commented 2023-07-27 09:07:54 +00:00
Author
Member
Copy link

This is the type of identifier (domain) for which order is generated on the client side. DNS and NNS, as far as I know, have the same domain name structure, so it is impossible to distinguish them

This is the type of identifier (domain) for which order is generated on the client side. DNS and NNS, as far as I know, have the same domain name structure, so it is impossible to distinguish them
👍 1
pogpp reviewed 2023-07-26 08:32:04 +00:00
acme/challenge.go Outdated
@ -49,6 +49,7 @@ const (
TLSALPN01 ChallengeType = "tls-alpn-01"
// DEVICEATTEST01 is the device-attest-01 ACME challenge type
DEVICEATTEST01 ChallengeType = "device-attest-01"
NNS01 ChallengeType = "nns-01"
pogpp commented 2023-07-26 08:32:04 +00:00
Member
Copy link

can you add a comment here?

can you add a comment here?
pogpp reviewed 2023-07-26 08:32:59 +00:00
authority/provisioner/acme.go Outdated
@ -26,6 +26,7 @@ const (
TLS_ALPN_01 ACMEChallenge = "tls-alpn-01"
// DEVICE_ATTEST_01 is the device-attest-01 ACME challenge.
DEVICE_ATTEST_01 ACMEChallenge = "device-attest-01"
NNS_01 ACMEChallenge = "nns-01"
pogpp commented 2023-07-26 08:32:59 +00:00
Member
Copy link

and comment here

and comment here
mbiryukova force-pushed feature/add_nns_challenge from 4cbe9ebb66 to 91f372b7b9 2023-07-26 12:53:44 +00:00 Compare
mbiryukova force-pushed feature/add_nns_challenge from 91f372b7b9 to 4630fe2cf2 2023-07-26 13:41:59 +00:00 Compare
alexvanin reviewed 2023-07-27 06:47:26 +00:00
acme/challenge.go Outdated
@ -503,0 +509,4 @@
nns := NNS{}
// TODO: retrieve NNS server URL from config
err := nns.Dial("http://localhost:30333")
alexvanin commented 2023-07-27 06:47:02 +00:00
Owner
Copy link

There is an issue with Dial: it does not close connection. See TrueCloudLab/frostfs-sdk-go#69

Let's modify nns package and add some sort of a Close() method and call it there with defer.

There is an issue with `Dial`: it does not close connection. See https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/issues/69 Let's modify `nns` package and add some sort of a `Close()` method and call it there with `defer`.
alexvanin marked this conversation as resolved
acme/challenge.go Outdated
@ -503,0 +511,4 @@
// TODO: retrieve NNS server URL from config
err := nns.Dial("http://localhost:30333")
if err != nil {
panic(err)
alexvanin commented 2023-07-27 06:39:29 +00:00
Owner
Copy link

Can we return error here instead of panic? Network might be unstable, so it is kinda "expected" error to process.

Can we return error here instead of panic? Network might be unstable, so it is kinda "expected" error to process.
alexvanin marked this conversation as resolved
go.mod Outdated
@ -47,0 +56,4 @@
golang.org/x/sync v0.3.0 // indirect
)
require (
alexvanin commented 2023-07-26 08:56:54 +00:00
Owner
Copy link

Is it formatted by go mod command? Can we keep single require section?

Is it formatted by `go mod` command? Can we keep single `require` section?
alexvanin marked this conversation as resolved
alexvanin commented 2023-07-27 06:50:48 +00:00
Owner
Copy link

Also, commits from TrueCloudLab members usually include:

Also, commits from TrueCloudLab members usually include: - header, starting with `[#xxx]` where `xxx` is an issue number or PR number (1 in this case) - a sign-off footer (`git commit -s`), see https://docs.pi-hole.net/guides/github/how-to-signoff/
mbiryukova force-pushed feature/add_nns_challenge from 4630fe2cf2 to 6af99af9d3 2023-07-27 14:01:13 +00:00 Compare
mbiryukova force-pushed feature/add_nns_challenge from 6af99af9d3 to 92a4884f2d 2023-07-28 08:05:37 +00:00 Compare
alexvanin approved these changes 2023-07-28 09:48:54 +00:00
acme/nns.go Outdated
@ -0,0 +36,4 @@
invoker interface {
Call(contract util.Uint160, operation string, params ...any) (*result.Invoke, error)
}
alexvanin commented 2023-07-28 09:48:31 +00:00
Owner
Copy link

I suggest to simplify code a little bit and drop invoker interface. We use client for all actions here: to init connection, to close it. Move Call method to multiSchemeClient interface, it will be much easier to read, I think.

I suggest to simplify code a little bit and drop `invoker` interface. We use `client` for all actions here: to init connection, to close it. Move `Call` method to `multiSchemeClient` interface, it will be much easier to read, I think.
alexvanin marked this conversation as resolved
mbiryukova force-pushed feature/add_nns_challenge from 92a4884f2d to f4dfed3bf3 2023-07-31 07:38:45 +00:00 Compare
alexvanin merged commit f4dfed3bf3 into tcl/master 2023-07-31 11:24:31 +00:00
alexvanin deleted branch feature/add_nns_challenge 2023-07-31 11:24:31 +00:00
alexvanin referenced this pull request from a commit 2023-07-31 11:24:32 +00:00
[#1] Add NNS Challenge support
realloc commented 2023-08-02 14:47:18 +00:00
Owner
Copy link

It would be nice to describe NNS-01 challenge as it's done for DNS Challenge in RFC8555.

It would be nice to describe NNS-01 challenge as it's done for [DNS Challenge](https://datatracker.ietf.org/doc/html/rfc8555#section-8.4) in RFC8555.
Sign in to join this conversation.
Reviewers
No reviewers
pogpp
alexvanin
Labels
Clear labels
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/certificates#1
No description provided.
Delete branch "mbiryukova/certificates:feature/add_nns_challenge"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?