Bill,
A few comments—
1. Should SecureRandom be seeded with something? An invariant but
machine-specific seed? I'm not sure, and I don't know what the
default seed is or whether a default seed weakens it somehow.
2. Isn't there already a toHex() somewhere in common-util? Seems
like a shame to not put it into a shared place and/or use one we
already have.
3. Just a weird minor flaw not peculiar to your code:
postConstruct() can be called by anything at any time, since it's
public. So even a mistake is a problem and could defeat the local
password until the servew is restarted: the code as written
overwrites the in-memory password, which would then differ from the
one on disk. Probably this should be more defensive.
4. What does XXX mean? Unresolved questions in the code are worrisome.
Lloyd
On Aug 13, 2009, at 11:36 AM, Bill Shannon wrote:
> I've implemented a fix for this issue:
> https://glassfish.dev.java.net/issues/show_bug.cgi?id=8699
>
> I described the approach in a message to arch_at_glassfish:
>
>> Currently in v3 the asadmin stop-domain command fails if the domain
>> has a non-default admin user and you don't specify the user name.
>> In v2 stop-domain would succeed in this case, as long as you were
>> on the same machine as the DAS.
>>
>> The v3 stop-domain command needs to execute the command on the server
>> (as opposed to using "kill" or something like that), which means it
>> needs to authenticate with the server. To allow v3 to be more
>> compatible
>> with v2, we're considering adding a new authentication mechanism that
>> will "only" work in the local case.
>>
>> Roughly, here's how this would work...
>>
>> On server startup, the server would generate a large random number
>> and write it in a file that is readable only by the owner of the
>> file (the user who started the server).
>>
>> Local commands, such as stop-domain, would read this file if it's
>> available and send the number as part of the authentication
>> information
>> to the server. The server would accept either the normal username/
>> password
>> authentication, or some special username along with this number as
>> the
>> password.
>>
>> This allows anyone who can read the file to authenticate to the
>> server.
>> Normally this would only be the user who owns the server and is
>> running
>> on the same machine.
>
> I'd like someone to review the server side code for this change.
>
> Here's the changes to GenericAdminAuthenticator to add this support,
> and the new LocalPassword class that implements this support:
>
> Index: GenericAdminAuthenticator.java
> ===================================================================
> --- GenericAdminAuthenticator.java (revision 30366)
> +++ GenericAdminAuthenticator.java (working copy)
> @@ -1,7 +1,7 @@
> /*
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
> *
> - * Copyright 1997-2008 Sun Microsystems, Inc. All rights reserved.
> + * Copyright 1997-2009 Sun Microsystems, Inc. All rights reserved.
> *
> * The contents of this file are subject to the terms of either the
> GNU
> * General Public License Version 2 only ("GPL") or the Common
> Development
> @@ -68,6 +68,10 @@
>
> @Inject
> volatile AdminService as;
> +
> + @Inject
> + LocalPassword localPassword;
> +
> private final Logger logger = Logger.getAnonymousLogger();
>
> public boolean login(String user, String password, String realm)
> throws
> LoginException {
> @@ -92,6 +96,11 @@
> if (anonok) {
> return anonok;
> }
> +
> + boolean isLocal = isLocalPassword(user, password);
> + if (isLocal)
> + return true;
> +
> try {
> AuthRealm ar = as.getAssociatedAuthRealm();
> if (FileRealm.class.getName().equals(ar.getClassname())) {
> @@ -141,6 +150,21 @@
> }
> return false;
> }
> +
> + /**
> + * Check whether the password is the local password.
> + * We ignore the user name but could check whether it's
> + * a valid admin user name.
> + */
> + private boolean isLocalPassword(String user, String password) {
> + if (!localPassword.isLocalPassword(password)) {
> + logger.finest("Password is not the local password");
> + return false;
> + }
> + logger.fine("Allowing access using local password");
> + return true;
> + }
> +
> public Subject authenticate(Object credentials) {
> if (this.serverAllowsAnonymousFileRealmLogin()) {
> logger.fine("Allowing anonymous login for JMX Access");
> @@ -159,6 +183,7 @@
> if (realm == null)
> realm = as.getAuthRealmName();
>
> + // XXX - allow local password for JMX?
> try {
> this.login(u, p, realm);
> return null; //for now;
>
>
> -----
>
> LocalPassword.java (minus the copyright info):
>
> package com.sun.enterprise.container.common;
>
> import java.io.*;
> import java.util.logging.*;
> import java.security.SecureRandom;
>
> import org.jvnet.hk2.annotations.Inject;
> import org.jvnet.hk2.annotations.Service;
> import org.jvnet.hk2.annotations.Scoped;
> import org.jvnet.hk2.component.PostConstruct;
> import org.jvnet.hk2.component.Singleton;
> import org.glassfish.api.admin.ServerEnvironment;
> import com.sun.enterprise.util.SystemPropertyConstants;
>
> /**
> * Manage a local password, which is a cryptographically secure
> random number
> * stored in a file with permissions that only allow the owner to
> read it.
> * A new local password is generated each time the server starts. The
> * asadmin client can use it to authenticate when executing local
> commands,
> * such as stop-domain, without the user needing to supply a password.
> *
> * @author Bill Shannon
> */
> @Service
> @Scoped(Singleton.class) // only want one local password
> public class LocalPassword implements PostConstruct {
>
> @Inject
> ServerEnvironment env;
>
> private String password;
>
> private static final String LOCAL_PASSWORD_FILE = "local-password";
> private static final int PASSWORD_BYTES = 20;
> private static final char[] hex = {
> '0', '1', '2', '3', '4', '5', '6', '7',
> '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'
> };
>
> private final Logger logger = Logger.getAnonymousLogger();
>
> /**
> * Generate a local password and save it in the local-password
> file.
> */
> public void postConstruct() {
> logger.fine("Generating local password");
> SecureRandom random = new SecureRandom();
> byte[] pwd = new byte[PASSWORD_BYTES];
> random.nextBytes(pwd);
> password = toHex(pwd);
> File localPasswordFile =
> new File(env.getConfigDirPath(), LOCAL_PASSWORD_FILE);
> PrintWriter w = null;
> try {
> if (!localPasswordFile.exists()) {
> localPasswordFile.createNewFile();
> /*
> * XXX - There's a security hole here.
> * Between the time the file is created and the
> permissions
> * are changed to prevent others from opening it,
> someone
> * else could open it and wait for the data to be
> written.
> * Java needs the ability to create a file that's
> readable
> * only by the owner; coming in JDK 7.
> */
> localPasswordFile.setWritable(false, false); // take
> from all
> localPasswordFile.setWritable(true, true); // owner
> only
> localPasswordFile.setReadable(false, false); // take
> from all
> localPasswordFile.setReadable(true, true); // owner
> only
> }
> w = new PrintWriter(localPasswordFile);
> w.println(password);
> } catch (IOException ex) {
> // ignore errors
> logger.log(Level.FINE, "Exception writing local password
> file", ex);
> } finally {
> if (w != null)
> w.close();
> }
> }
>
> /**
> * Is the given password the local password?
> */
> public boolean isLocalPassword(String p) {
> return password != null && password.equals(p);
> }
>
> /**
> * Convert the byte array to a hex string.
> */
> private String toHex(byte[] b) {
> char[] bc = new char[b.length * 2];
> for (int i = 0, j = 0; i < b.length; i++) {
> byte bb = b[i];
> bc[j++] = hex[(bb >> 4) & 0xF];
> bc[j++] = hex[bb & 0xF];
> }
> return new String(bc);
> }
> }
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>
Lloyd Chambers
lloyd.chambers_at_sun.com
GlassFish Team