feat: #2246 Support for DID Web Method #2288
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2288 +/- ##
==========================================
- Coverage 89.48% 89.46% -0.03%
==========================================
Files 223 228 +5
Lines 15885 15928 +43
==========================================
+ Hits 14215 14250 +35
- Misses 985 989 +4
- Partials 685 689 +4
Continue to review full report at Codecov.
|
|
addressed changes requested to test urls and tidied up the did parsing code, working on making the http client more robust now as per your recommendations |
|
This pull request introduces 1 alert when merging 866fc34 into e8a7705 - view on LGTM.com new alerts:
|
pkg/vdr/web/did.go
Outdated
| } | ||
|
|
||
| switch pathComponents[0] { | ||
| case "localhost", "127.0.0.1": |
There was a problem hiding this comment.
Is this code added just for testing purposes? If so you should elaborate why because we usually don't add extra code to support testing.
Is this valid scenario for "web" VDR?
There was a problem hiding this comment.
yes it's just for testing, web dids shouldn't be originating from any site with a host:port url really as any organization trusted enough to have did:web would likely have a proper url
There was a problem hiding this comment.
from the spec: "The method specific identifier MUST match the common name used in the SSL/TLS certificate, and it MUST NOT include IP addresses or port numbers. Directories and subdirectories MAY optionally be included, delimited by colons rather than slashes. " https://w3c-ccg.github.io/did-method-web/#method-specific-identifier
There was a problem hiding this comment.
switch pathComponents[0] {
// This is for testing purposes ONLY and should not be used in production
case "localhost", "127.0.0.1":
urlString = "https://" + strings.Join(pathComponents[0:2], ":") + "/" + strings.Join(pathComponents[2:], "/")
log.New("aries-framework/pkg/vdr/web").Warnf("using an ip address or port number does not conform to spec")
would something like this be acceptable ? @sandrask
There was a problem hiding this comment.
This is not acceptable.
Yes, 127.0.0.1 is not allowed by the spec, but localhost is.
Also, ports will be supported. There is a proposed solution to base64url-encode the host portion: w3c-ccg/did-method-web#7 (comment). At least one implementation has adopted the proposal: https://github.com/ProjectProvenance/proof-points/pull/42/files.
@amodkala let's adopt the proposal - assume ports are allowed and the host:port portion is base64url-encoded.
There was a problem hiding this comment.
@amodkala my apologies - somehow I mixed up url-encoding with base64url-encoding this morning. The proposal for the did:web ID to have the host:port portion and all path components url-encoded. See the issue I linked to above for more details.
There was a problem hiding this comment.
Just realized url-encoding the path components makes it non-compliant with the generic did-core syntax
pkg/vdr/web/did.go
Outdated
| } | ||
|
|
||
| pathComponents := strings.Split(parsedDID.MethodSpecificID, ":") | ||
| pathComponents[0], err = url.QueryUnescape(pathComponents[0]) |
There was a problem hiding this comment.
We should actually do this in did.Parse() (#2336).
pkg/vdr/web/resolver.go
Outdated
| return nil, fmt.Errorf("error resolving did:web did --> could not parse did:web did --> %w", err) | ||
| } | ||
|
|
||
| resp, err := docOpts.HTTPClient.Get(url) |
There was a problem hiding this comment.
If the http client is configured properly, it will validate the certificate against the configured root authorities. This is good.
Is this also ensuring this part of the did:web spec (important):
The method specific identifier MUST match the common name used in the SSL/TLS certificate, and it MUST NOT include IP addresses or port numbers.
So for instance if you resolve did:web:acme.org and the server's certificate's common name is "Faber College" - does it fail or not? This might be a followup PR - can you confirm?
There was a problem hiding this comment.
After some digging through the http and tls packages I don't think we explicitly check for that at the moment. Would this just require a comparison between the two (common cert name and method specific identifier)?
There was a problem hiding this comment.
Would this just require a comparison between the two (common cert name and method specific identifier)?
Roughly, yes.
There was a problem hiding this comment.
@amodkala are we updating this PR any time soon? Or would we rather add this verification in a followup?
pkg/vdr/web/resolver_test.go
Outdated
| require.Contains(t, err.Error(), "error parsing did doc") | ||
| }) | ||
| /* | ||
| t.Run("test resolve did with certificate mismatch failure", func(t *testing.T) { |
There was a problem hiding this comment.
@amodkala can we enable this test?
Signed-off-by: Amod Kala <[email protected]>
Adds ability to resolve did:web dids, addresses issue #2246
Signed off by: Amod Kala [email protected]