Skip to content

crypto/cryptoJava: Add support for parsing Subject Alternative Names …#91

Open
rnschwalm wants to merge 2 commits intofantom-lang:masterfrom
rnschwalm:subjectAlternativeName
Open

crypto/cryptoJava: Add support for parsing Subject Alternative Names …#91
rnschwalm wants to merge 2 commits intofantom-lang:masterfrom
rnschwalm:subjectAlternativeName

Conversation

@rnschwalm
Copy link
Copy Markdown
Contributor

…from CSRs and X509 certificates

Copy link
Copy Markdown
Member

@mgiannini mgiannini left a comment

Choose a reason for hiding this comment

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

A few small things to clean up.

Comment thread src/crypto/fan/San.fan
// Construction
//////////////////////////////////////////////////////////////////////////

static San dns(Str name) { San(SanType.dNSName, name) }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All these convenience methods should be static constructors

static new dns(Str name) { ... }
static new email(Str email) { ... }
etc.

Comment thread src/crypto/fan/San.fan
static San ip(Obj ip)
{
IpAddr := Type.find("inet::IpAddr")
if (ip is Str) return San(SanType.iPAddress, IpAddr.make([(Str)ip]))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you don't need this cast. You can just call make as:

IpAddr.make([ip])

Comment thread src/crypto/fan/San.fan
if (((Uri)uri).isRel) throw ArgErr("Parameter must not be a relative Uri")
return San(SanType.uniformResourceIdentifier, uri.toStr)
}
else if (uri is Str)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is duplicate code. if uri is a Str you should call uri again with a Uri:

else if (uri is Str) return uri(Uri.fromStr(uri))

Comment thread src/crypto/fan/San.fan
this.val = val
}

static new fromValue(Obj value)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you at least need to document this behavior for Str (i.e. it assumes DNS (and not email, or other Str type)). Probably the docs you have in CertSigner should be here ,and just reference this method for the suported convenience values


private static AsnObj toAsn(San san)
{
tagId := ((SanType)san.type).tagId
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think this cast is unnecessary

tagId := san.type.tagId

case SanType.rfc822Name:
case SanType.dNSName:
case SanType.uniformResourceIdentifier:
return Asn.tag(AsnTag.context(tagId).implicit).str((Str)(san.val), AsnTag.univIa5Str)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this (Str) cast is unnecessary

.str(san.val) should work

return Asn.tag(AsnTag.context(tagId).implicit).octets(((IpAddr)san.val).bytes)

case SanType.registeredID:
return Asn.tag(AsnTag.context(tagId).implicit).oid((Str)san.val)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this (Str) cast is unnecessary

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.

2 participants