Skip to content

Conversation

@cljk
Copy link

@cljk cljk commented Jul 23, 2019

Currently I´m trying to write a Linux shadow-file - the passwords are stored as Base64-ecoded (hashes). But the alphabet of this Base64 is different:

see www.linuxquestions.org/questions/linux-security-4/how-can-i-convert-a-sha-512-etc-shadow-hash-to-base64-4175477045/
A base 64 encoding is used to store password hashes computed with crypt in the /etc/passwd. Its alphabet starts with '.' for zero, then '/' for one, followed by 0-9, A-Z and a-z. Padding is not used.

So I opened up your Base64 class constructor to accept an alphabet.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 92.74% when pulling c44f596 on cljk:master into 47a55d2 on apache:master.

@coveralls
Copy link

coveralls commented Jul 23, 2019

Coverage Status

Coverage increased (+0.02%) to 92.803% when pulling 3e0d13c on cljk:master into 47a55d2 on apache:master.

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

This PR needs unit tests ;-)

@cljk
Copy link
Author

cljk commented Jul 23, 2019

You´re right.
I added one unit test to check the encoding output of using a custom alphabet by comparing it to the default alphabet output.

I made the byte arrays of the alphabets public. Not just because I needed it for the unit test but I think that the encoding table might be necessary elsewhere in other projects?!
If this is not wanted I´ll copy the array to the test-case...

When my personal project with the shadow table is finished I´d like to add the crypt table.

@cljk
Copy link
Author

cljk commented Jul 23, 2019

Hm.... I´ve no idea why the travis build fails.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 58.236 s
[INFO] Finished at: 2019-07-23T20:05:09Z
[INFO] Final Memory: 47M/160M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.0:javadoc (default-cli) on project commons-codec: An error has occurred in Javadoc report generation: 
[ERROR] Exit code: 1 - javadoc: error - The code being documented uses modules but the packages defined in https://docs.oracle.com/javase/7/docs/api/ are in the unnamed module.
[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://docs.oracle.com/javaee/6/api/ are in the unnamed module.
[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://docs.oracle.com/javase/7/docs/api/ are in the unnamed module.
[ERROR] 
[ERROR] Command line was: /home/travis/oraclejdk11/bin/javadoc @options @packages
[ERROR] 
[ERROR] Refer to the generated Javadoc files in '/home/travis/build/apache/commons-codec/target/site/apidocs' dir.
[ERROR] 

I work on Corretto JDK 11 and my maven build succeeds?!

mvn clean package site

works on my system.

@cljk
Copy link
Author

cljk commented Jul 23, 2019

Okay... can reproduce the error - but it´s no fault in my changes.
Same error occurs in original code.

@aherbert
Copy link
Contributor

I think the build fails because the maven-javadoc-plugin configuration section in build does not have this:

<source>${maven.compiler.source}</source>

The plugin is configured in commons-parent. It has this tag for the reporting section and the release profile but not for the build section.

@cljk Can you confirm that you cannot do this with Corretto JDK 11:

mvn javadoc:javadoc

or default goal

mvn

This should work:

mvn javadoc:javadoc -Dsource=1.7

On the code change, do not make the array public. The values are mutable. Copy the array to the unit test or provide a package private level static getter which returns a clone of the array.

You should also change the public constructor that accepts an input alphabet to check it has 64 characters. I'm not sure where we are with defensive copying of input arrays so I'll leave that judgement for someone else.

@cljk
Copy link
Author

cljk commented Jul 23, 2019

@aherbert
Yes, I can confirm:

mvn javadoc:javadoc

fails, mvn too and

mvn javadoc:javadoc -Dsource=1.7

works...

But perhaps we should open an issue for that since that´s not caused by my pull request?!


  • added check for encodeTable length ... and a unit test for this check
  • encode tables private again
  • reintruduced(?) usage of local variable decodeTable - needed for successful decoding of data encoded with custom encoding table
    • for STANDARD_ENCODE_TABLE or URL_SAFE_ENCODE_TABLE it uses the precalculated DEFAULT_DECODE_TABLE as before
    • for custom encode tables: added a calculation method for decode tables ... and an encode/decode test for a modified encode table

" defensive copying of input arrays" - If you think making a duplicate of the encodeTable improves security in any way (?)... adding this would be a one-liner?!

@kkocel
Copy link
Contributor

kkocel commented Apr 16, 2024

@garydgregory this can be closed, as my PR got merged :)

@garydgregory
Copy link
Member

Closing since #266 is merged.

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.

5 participants