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

Allow to extend X509 extensions #1573

Merged

Conversation

kylekatarnls
Copy link
Contributor

the X509ExtensionTest.php file below is an example of the goal I try to achieve with this pull-request:

Have custom extensions (as per RFC 3280) with some meta-data to be ASN1-encoded alongside with extensions natively supported by phpseclib.

From what I could try until now, without this little change, the ASN1 encoder has no way to know the structure of an extension if it's not in the list of supported ones.

@terrafrost
Copy link
Member

I'll try to take a look at this and your other PRs this weekend - thanks for all your work!!

@terrafrost
Copy link
Member

So I guess one thing I'm wondering is... why use a trait if it's only going to be used by one file?

This could also be extended to CSR's, CRL's, etc, by making use of the private function &extensions(&$root, $path = null, $create = false) method.

That said, this does (at least currently for X509) eliminate the need for the obnoxious resigning that is currently required, which I like!:

https://phpseclib.com/docs/x509#example-custom-extensions

It might not be a bad idea to throw an exception if an extension is already registered in registerExtension. eg. do an if (isset(self::$extensions[$id])) { throw \RuntimeException('Extension ' . $id . ' has already been defined'); } or some such.

Otherwise, I like it!

I'll hold off for merging for the time being to let you make these changes. I suppose I could make them myself but it'd make my life easier if you could do them 😅

@kylekatarnls
Copy link
Contributor Author

I made a trait, because the extensions arrays and setters are standalones, applicable on similar extendable mappings, and to avoid bloating much more the X509.php. But I see it's yet very big, maybe you don't care too much about splitting responsibilities?

About the exception. I would be fully agree if ASN1::$oids (and so $extensions) were not static. So it would be fair you handle an instance and can't register twice on it. But in the many contexts it can apply, to work with a custom extension you would have no choice to register it globally. If 2 libraries foo and bar implements the same custom extension (following the same specifications) they will work smoothly using only one or the other in an application but not both if an exception is thrown while they would actually be compatible (assuming those libraries have more code to run after the registerExtension() call, user-land will have no way to properly catch it.

It will also make more painful unit testing when using such custom extension.

Are you OK to not throw the exception if the already registered extension is exactly the same and also provide a getter so you can check what is already registered?

@terrafrost
Copy link
Member

I made a trait, because the extensions arrays and setters are standalones, applicable on similar extendable mappings, and to avoid bloating much more the X509.php. But I see it's yet very big, maybe you don't care too much about splitting responsibilities?

I do but not for 3.0. For 4.0 I want to completely redo how X509 works. Separate classes for X509, CSR, CRL, etc. Whereas right now loading an X509 returns an array with what I'm thinking for 4.0 you'll just do maybe $x509 = new X509; $x509->load(...); print_r($x509['tbsCertificate']);. ie. X509 will be an instance of ArrayObject. And if you don't do $x509->load(...) you'll still be able to make changes to the X509 structure before saving it because it'll by default be a minimally viable X509 cert.

But for 3.0 I'd just assume stick with the bloat. If we're gonna have a trait for this why not for other stuff? I think consistency is important and having a trait for one thing but for nothing else just isn't consistent.

Are you OK to not throw the exception if the already registered extension is exactly the same and also provide a getter so you can check what is already registered?

Works for me!

@kylekatarnls
Copy link
Contributor Author

  • Added getRegisteredExtension()
  • Moved trait inside X509 class
  • Added exception on mapping conflict

@terrafrost terrafrost merged commit 3e32d5a into phpseclib:3.0 Jan 16, 2021
@kylekatarnls kylekatarnls deleted the feature/allow-to-use-extensions branch January 16, 2021 12:28
@kylekatarnls
Copy link
Contributor Author

Thanks @terrafrost and thanks for this library 🙏

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.

None yet

2 participants