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

Unified, no more logic than dispatch in SecDispatcher, and is self-describable #75

Merged
merged 27 commits into from
Oct 14, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Oct 9, 2024

Hence, moved version to 4.x.

Changes:

  • "master pw" is really just one (of possibly multiple) dispatchers, so moved "master password" into dispatchers. Now SecDispatcher is really just a "dispatcher" with one Dispatcher implementation OOTB
  • similarly, old sec behaviour is moved to dispatcher as "legacy" to support mvn3 encrypted passwords (only decrypting)
  • dispatcher passwords now minimally include "name" attribute (and more, if dispatcher adds it), master dispatcher adds "cipher" as well, this allows future proof working if new Cipher added and user makes it default: non-reencoded passwords will still work with old cipher.
  • Dispatcher and MasterSource interfaces have corresponding "meta" interfaces where they can "describe themselves"
  • added support for pinentry as well
  • configuration validation now performs "deep validation" and produces useful report
  • this is no more OOTB ready-to-use JSR330 component. As we know from history, it WAS before, and it provided a "bad" default configuration path that was NEVER used, and forced Maven instead to always redefine the component to make it usable within Maven (to make it use maven expected config). So, it is now the integrator duty to "properly integrate" this component, recommended way is to create JSR330 Provider with it. Also, the "system property override" of configuration path was removed, whatever integrating app needs is doable from it's own Provider implementation.

Self describe and validation in action:
https://asciinema.org/a/9kmtQWhKJC9elFp3tiDlxpOTE

Also introduce two "test" ones, one dispatcher and
one source, usable ONLY for testing, as those two
have nothing with "encryption".
@cstamas cstamas self-assigned this Oct 9, 2024
@cstamas cstamas changed the title Self describe Yet another non compatible change Oct 10, 2024
}

public static void main(String[] args) throws IOException {
Result<String> pinResult = new PinEntry("/usr/bin/pinentry-gnome3")
Copy link
Member

Choose a reason for hiding this comment

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

on macos we have:

% which pinentry                                          
/opt/homebrew/bin/pinentry

% which pinentry-mac 
/opt/homebrew/bin/pinentry-mac

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, change the executable and just run main from IDE to see it in action 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

My original goal was to make it work with https://github.com/jorgelbg/pinentry-touchid (so maven encryption with touchID on mac, yay!) but the project seems dead or unsure, but the app did not work on my mac with macOS 15.0.1

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, this is the important bit:

.setKeyInfo("maven:masterPassword")

as this is the "handle" for sensitive data, so this is the name of your master pw in keychain, if it is in keychain. According to pinentry doco, this is usually either keyGrip (for PGP keys) or "app:something" formed id.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, this is how my settings-security.xml looks like:

<?xml version='1.0' encoding='UTF-8'?>
<settingsSecurity xmlns="http://codehaus-plexus.github.io/plexus-sec-dispatcher/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://codehaus-plexus.github.io/plexus-sec-dispatcher/4.0.0 https://codehaus-plexus.github.io/xsd/plexus-sec-dispatcher-4.0.0.xsd">
  <modelVersion>4.0</modelVersion>
  <defaultDispatcher>master</defaultDispatcher>
  <configurations>
    <configuration>
      <name>master</name>
      <properties>
        <property>
          <name>source</name>
          <value>pinentry-prompt:/usr/bin/pinentry-gnome3</value>
        </property>
        <property>
          <name>cipher</name>
          <value>AES/GCM/NoPadding</value>
        </property>
      </properties>
    </configuration>
  </configurations>
</settingsSecurity>

As you guessed, format is pinentry-prompt:$pathToPinentry. Just make yours like this, or use mvnenc init and then edit it manually (as mvnenc is not yet fully done).

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I do not. mvn3 encryption is so badly broken, I can share the details in private, but I have no intent to make it backward compat.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead, we may want to upgrade sec-dispatcher to 4.0 in mvn 3.10? But we would lose "mvnenc init" (use mvn4 for that), and also --encrypt-master-password as well...

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think about add a backward compatibility ...

Oh, now I realized: mvn3 would fail to load the new format... so maybe we need new file name / path for sec-dispatcher 4?

Copy link
Member Author

Choose a reason for hiding this comment

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

Radically simplifies this library, removed all mumbo jumbo about default config file, and Java System property overrides, it is now purely the duty of integrating app how will this be integrated. Also, integrating app "knows" what it needs, and using globals like Java system properties is anyway something is not nice from library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Backward compat solved @slawekjaranowski Now mvn4 will warn if legacy encrypted pw found, and am adding new goal "migrate". If you use mvn3 and mvn4 on same workstation and you insist on encryption, you must use the (broken) mvn3 encryption, and live with warnings mvn4 will emit.

Do not provide "bad" and "never used" defaults as before as it
was just the source of confusion, as old component had to be
"redefined" in Plexus XML or alike anyway, to be usable in Maven.
@cstamas cstamas marked this pull request as ready for review October 11, 2024 17:17
pom.xml Show resolved Hide resolved
@cstamas cstamas changed the title Yet another non compatible change Unified, no more logic than dispatch in SecDispatcher, and is self-describable Oct 12, 2024
@slachiewicz
Copy link
Member

Why not have this all sec-dispatcher part of/module inside Maven 4 ? With having possibility to extend/add own encoders if needed as extensions? I don't think this component will be used outside of mvn anyway.

@cstamas
Copy link
Member Author

cstamas commented Oct 12, 2024

This lib will not change a lot, it had now a disturbance as we reworked all the bad parts of this (and cipher), and I envision next change (to cipher) when CodeQL declares currently used Cipher "unsafe" (like it did with existing Cipher in 2.0) and we will need to migrate to even newer thing. This is good to have it here.

@cstamas
Copy link
Member Author

cstamas commented Oct 13, 2024

@slachiewicz @slawekjaranowski @gnodet pls review, I want to release this soon, and then merge the mvn PR as well:
apache/maven#1793

Copy link
Member

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

  • I'm not convinced by the short attributes, is space so much an issue that we need one-char attributes ?
  • the 5 MasterSource and 2 Dispatcher implementations still have jsr330 annotations

Space is not a problem, these passwords are in Maven settings
only, so "space saver short attributes" are meaningless.
@cstamas cstamas merged commit ab0942e into master Oct 14, 2024
17 checks passed
@cstamas cstamas deleted the self-describe branch October 14, 2024 10:38
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.

4 participants