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

add support for deploymentruleset #816

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

dhirenjoshi
Copy link

No description provided.

@sclassen sclassen changed the title pushing changes to dhirenjoshi/IcedTea-Web/deploymentruleset-add branch add support for deploymentruleset Aug 5, 2021
@sclassen
Copy link
Contributor

sclassen commented Aug 5, 2021

Hi Dhiren

Thank you for this contribution.
I see one main problem which you need to address before we can merge this. Your Change adds 5 new dependencies to icedtea-web. This is problematic as all these dependencies will end on the classpath of each and every application launched with icedtea-web. Therefor the more dependencies icedtea-web has the bigger is the risk of conflicting dependencies with an application.

If you look at the XmlParser and the Parser class you see that so far icedtea-web is using classes included in the JRE to parse the JNLP file, which is also an XML file.

Can you change your code in such a way that it no longer requires new dependencies.

@dhirenjoshi
Copy link
Author

dhirenjoshi commented Aug 5, 2021 via email

@sclassen
Copy link
Contributor

sclassen commented Aug 5, 2021 via email

Removed reference to jaxb . Only plain XML parsing is being done now.
@dhirenjoshi
Copy link
Author

dhirenjoshi commented Aug 12, 2021 via email

@dhirenjoshi
Copy link
Author

dhirenjoshi commented Aug 16, 2021 via email

@sclassen
Copy link
Contributor

Hi Dhiren

I finally found some time to look into the code.
I took the liberty to delete a lot of code which was not reachable. Also many of your comments and log statements.

I hope you are not intrigued by this. If you want I can always restore your original code.

Regarding the Ruleset implementation:
1)
I am not sure how you handle the case with no ruleset. This should still be the default case. From my understanding of the code currently no ruleset -> empty list of rules -> empty list of URLs -> nothing is allowed.
2)
The current implementation is creating a logical disjunction with the whitelist from the deployment properties. But in most cases this white list will be empty and thus any URL will pass it. Maybe a logical conjunction over the two whitelists would be the correct approach.
3)
Also I am not sure why you added a configuration option since the Oracle specification is very clear on where the file need to be located on the file system (see https://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/deployment_rules.html#package)

There are also some implementation specific things to look at but let us first tackle the conceptual points mentioned above.

@dhirenjoshi
Copy link
Author

dhirenjoshi commented Aug 31, 2021 via email

@sclassen sclassen marked this pull request as draft September 10, 2021 14:19
@sclassen
Copy link
Contributor

Hi Dhiren, I read most of the specification from https://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/deployment_rules.html

There are still a few crucial parts missing in the implementation.
I can support you in implementing this but I cannot do all the remaining work by myself.

What are your plans with this feature? Do you have the time to continue on this?

@dhirenjoshi
Copy link
Author

dhirenjoshi commented Sep 10, 2021 via email

@sclassen
Copy link
Contributor

OK, so let's do this.

I was thinking a bit and I would like to propose the following:

  • I will open issues for the things which need improvement or are missing. Mainly to avoid a big chaos in this PR.
  • All code should go into the branch dhirenjoshi:deploymentruleset-add
  • I will also commit code to this branch but feel free to change all my code. My code is not perfect and I have absolutely no problems with you changing my code

But before we start one last question:
When reading the specification of the deploymentruleset I understand it that you define rules for RIA (rich internet applications) so the rules apply to the entire application and not the single resources they are composed of. So in my understanding if I add a rule for host.example.com then any RIA at this URL is allowed to run. And they can also download resources from opensource.someSite.net without me adding this second address to the ruleset.

So my question is: Can the ruleset be applied as a whitelist to the resources or should it be applied earlier (i.e. directly after parsing the JNLP file)

@dhirenjoshi
Copy link
Author

dhirenjoshi commented Sep 13, 2021 via email

@dhirenjoshi
Copy link
Author

dhirenjoshi commented Sep 30, 2021 via email

@sclassen
Copy link
Contributor

sclassen commented Oct 4, 2021

@dhirenjoshi see #830

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