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

DDF-6215 Upgrade to Karaf 4.2.9 #6216

Merged
merged 7 commits into from
Aug 21, 2020
Merged

DDF-6215 Upgrade to Karaf 4.2.9 #6216

merged 7 commits into from
Aug 21, 2020

Conversation

SmithJosh
Copy link
Contributor

@SmithJosh SmithJosh commented Aug 4, 2020

What does this PR do?

Upgrades to Karaf 4.2.9, and along with it upgrades a bunch of other dependencies to match the version that Karaf uses.

There was an attempt to upgrade previously, which ran into a number of issues (#5798). I ran into a few of the same issues for this PR. Here's the fixes:

Some change, somewhere, has led to a problem with the bnd library we use (stacktrace here). There are two versions that I can see in the system and neither seems to correctly parse some classfile.

This was a problem parsing one of the cxf feature definitions. I copied the fix from apache/cxf#602

Some change has apparently altered startup order such that when running component tests, many (perhaps all) of our filters cannot be injected because their target servlet contexts have already started.

I changed our DelegateServletFilter to be a Jetty Handler, so it doesn't need to be injected into each individual servlet context. There was no way I could find to access the servlet contexts before they were started. The handler approach accomplishes the same thing - running filters for all requests - but filters must now implement the org.codice.ddf.platform.filter.http.HttpFilter interface instead of javax.servlet.Filter (Jetty Handlers accept HttpServletRequests instead of ServletRequests).

This PR depends on: codice/thirdparty#97

Who is reviewing it?

@blen-desta @bakejeyner @garrettfreibott @stustison

Select relevant component teams:

@codice/security

How should this be tested?

Verify:

  • Full build with itests passes
  • Error pages are still registered. If you refresh the platform-paxweb-jettyconfig bundle, you should see a bunch of messages indicating that error pages are being injected as the webapps restart. Also, produce an error in the UI (e.g. 403 by hitting /admin as guest) and verify you see the expected DDF error page.
  • HttpFilters are run on all requests and after the SecurityFilters (WebSsoFilter, LoginFilter, etc.). You can turn up the logs for org.codice.ddf.pax.web.jetty to TRACE and you should see a message Delegating to 5 HttpFilters when you make a request.
  • The metrics filter still works. Hit /metrics and verify the ddf_platform_* metrics look realistic.
  • HttpFilters are run in the correct order: SecurityJavaSubjectFilter first, DoPrivilegedFilter last, anything else in between

Any background context you want to provide?

This is part of an ongoing effort to reduce the CVE level in DDF

What are the relevant tickets?

Fixes: #6215

Screenshots

Checklist:

  • Documentation Updated
  • Update / Add Threat Dragon models
  • Update / Add Unit Tests
  • Update / Add Integration Tests

Notes on Review Process

Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.

Review Comment Legend:

  • ✏️ (Pencil) This comment is a nitpick or style suggestion, no action required for approval. This comment should provide a suggestion either as an in line code snippet or a gist.
  • ❓ (Question Mark) This comment is to gain a clearer understanding of design or code choices, clarification is required but action may not be necessary for approval.
  • ❗ (Exclamation Mark) This comment is critical and requires clarification or action before approval.

Copy link
Member

@pklinef pklinef left a comment

Choose a reason for hiding this comment

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

FilterInjector and DelegateServletFilter are being used to hook in metrics to all servlets in the system using the ServletMetrics filter. I could not find a way to get the Pax Web whiteboard to hook into all servlets in the system which is why I just fixed the injector and delegate.
https://github.com/codice/ddf/blob/master/platform/metrics/metrics-servlet-filter/src/main/java/org/codice/ddf/metrics/servlet/ServletMetrics.java

I think we could support the use case with a custom Jetty Handler.
https://www.eclipse.org/jetty/documentation/current/jetty-handlers.html#writing-custom-handlers

The GZIP handler looks like a good example of connecting to all servlet requests and responses.
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/config/modules/gzip.mod
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/config/etc/jetty-gzip.xml

StatisticsHandler is a more direct example.
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/handler/StatisticsHandler.java

Looks like there is even a way to register handlers directly as OSGi services which would be easier then trying to figure out how to get a mod to work with Pax Web.
http://ops4j.github.io/pax/web/SNAPSHOT/User-Guide.html#using-handler-and-connectors-as-services

@SmithJosh
Copy link
Contributor Author

FilterInjector and DelegateServletFilter are being used to hook in metrics to all servlets in the system using the ServletMetrics filter. I could not find a way to get the Pax Web whiteboard to hook into all servlets in the system which is why I just fixed the injector and delegate.
https://github.com/codice/ddf/blob/master/platform/metrics/metrics-servlet-filter/src/main/java/org/codice/ddf/metrics/servlet/ServletMetrics.java

I think we could support the use case with a custom Jetty Handler.
https://www.eclipse.org/jetty/documentation/current/jetty-handlers.html#writing-custom-handlers

The GZIP handler looks like a good example of connecting to all servlet requests and responses.
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/config/modules/gzip.mod
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/config/etc/jetty-gzip.xml

Looks like there is even a way to register handlers directly as OSGi services which would be easier then trying to figure out how to get a mod to work with Pax Web.
http://ops4j.github.io/pax/web/SNAPSHOT/User-Guide.html#using-handler-and-connectors-as-services

@pklinef I had the same issue trying to get a whiteboard service to hook into all servlets. Pax Web doesn't support it. That's why I ended up with the solution in this PR. Services implementing the SecurityFilter interface do get applied to all requests, so I made the DelegateServletFilter into a DelegateSecurityFilter. It's a workaround, but it performs the intended behavior. And it appears that the Pax Web bug preventing us from hooking into all servlets via the whiteboard will be fixed in pax-web 8, so I thought that was good enough for now.

I can look into the Handler suggestion, but I'm a little unclear what the issue with this solution is

@SmithJosh
Copy link
Contributor Author

@pklinef I looked at the ServletMetrics filter and I think I understand your concern now. So since the delegate filter is now part of the SecurityFilter chain, it doesn't have access to the ServletFilter chain in the actual webapp that gets hit like it did before and can't accurately measure request latency. And indeed, I was able to confirm the latency metrics are wrong.

I'm looking into the Jetty Handler idea to address that. It sounds like your concern doesn't apply to the rest of the filters, so I'll just plan on changing the ServletMetrics filter for now.

Copy link
Contributor Author

@SmithJosh SmithJosh left a comment

Choose a reason for hiding this comment

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

@pklinef and @Lambeaux I believe I've addressed your concerns. Can you please re-review?

* <p>When https://ops4j1.jira.com/browse/PAXWEB-1123 is resolved, this workaround should be
* revisited.
*/
public class DelegatingHttpFilterHandler extends HandlerWrapper {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The delegating servlet filter has a new home 😂 This time it's the Jetty Handler chain

*/
public interface HttpFilter {

void doFilter(
Copy link
Contributor Author

@SmithJosh SmithJosh Aug 14, 2020

Choose a reason for hiding this comment

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

The Jetty Handlers take HttpServletRequests. In order to inject stuff into that chain, I needed to have a filter interface that takes HttpServletRequests as well. Oddly enough this is not already a thing.

Any filters we used to inject via the delegate filter will need to be changed to implement this new interface. That will require changing the method signature and removing the init/destroy methods since those aren't used anymore.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class ServletMetrics implements Filter {
public class ServletMetrics implements HttpFilter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an example of converting an existing Filter into an HttpFilter


private HttpFilter[] getFilters() {
HttpFilter[] filters = new HttpFilter[filterTracker.size()];
return filterTracker.getServices(filters);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is getting injected into the Jetty Handler chain via the jetty.xml config, so I couldn't just inject a service reference list like it did before. If anybody has better ideas for how to do this, I'm open to suggestions.

@bakejeyner
Copy link
Contributor

Hero ✔️

  • verified filters are re-injected after refreshing bundle
  • error page is displayed
  • filters are being injected, and the default http filters are being ran
  • metrics look fine
  • security filters run in correct order

@SmithJosh
Copy link
Contributor Author

build now

@cxddfbot
Copy link

Internal build has been started, your results will be available at build completion.

@cxddfbot
Copy link

Build SUCCESS See the job results in legacy Jenkins UI or in Blue Ocean UI.

@parret parret merged commit 7f7adf4 into codice:master Aug 21, 2020
jlcsmith pushed a commit to jlcsmith/ddf that referenced this pull request Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Karaf 4.2.9
8 participants