[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user asfgit closed the pull request at: https://github.com/apache/incubator-guacamole-client/pull/183 ---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r147473937
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.cas.conf;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.properties.GuacamoleProperty;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+FileInputStream keyStreamIn = new FileInputStream(keyFile);
+ByteArrayOutputStream keyStreamOut = new
ByteArrayOutputStream();
+byte[] keyBuffer = new byte[1024];
+try {
+for (int readBytes; (readBytes =
keyStreamIn.read(keyBuffer)) != -1;)
+keyStreamOut.write(keyBuffer, 0, readBytes);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("IOException while
trying to read bytes from file.", e);
--- End diff --
Ummm...I wasn't paying attention to the fact that I already had it there?!
Removed.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r147473323
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.cas.conf;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.properties.GuacamoleProperty;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+FileInputStream keyStreamIn = new FileInputStream(keyFile);
+ByteArrayOutputStream keyStreamOut = new
ByteArrayOutputStream();
+byte[] keyBuffer = new byte[1024];
+try {
+for (int readBytes; (readBytes =
keyStreamIn.read(keyBuffer)) != -1;)
+keyStreamOut.write(keyBuffer, 0, readBytes);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("IOException while
trying to read bytes from file.", e);
--- End diff --
"IOException while trying to read bytes" will only have meaning to a
developer. Any reason why this error is being caught here, rather than letting
it get caught by the identical catch which comes later? The message for that
other catch, "could not read in the specified key file", is much more clear.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r147463115
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
Okay, so in the interest of getting this prepped more quickly for the
0.9.14-incubating release, I've moved the property to the CAS extension, and
also implemented the the `ByteArrayOutputStream` solution instead of pulling in
the extra package and changing the version requirement.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144834201
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
I figured the property was generic enough that it might be useful to some
other purpose in the future, so I put it in the guacamole-ext area instead of
CAS. I can move it if it makes sense at the moment - it can always be moved
back up in the future if someone else finds it useful.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144761726
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
Oh.
Why is `PrivateKeyGuacamoleProperty` being added to the public extension
API, rather than being internal to the CAS extension?
I'm not necessarily against adding a new property type at the extension API
level, but PKCS8 + RSA is rather specific.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144711034
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
While stopping support for Java 6 entirely would need some discussion,
requiring Java 7 for a particular extension (such as the CAS auth) seems OK
from my perspective.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144708371
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
Java 7 has a `readAllBytes` method in its `java.nio.file.Files` class:
http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#readAllBytes(java.nio.file.Path)
However, I thought I had seen some things in the pom.xml files that
indicated that we try to maintain Java 6 compatibility, so I avoided that
route. If I'm mistaken about that or we're willing to put a Java 7 requirement
on it, we could use the built-in and avoid both writing our own and pulling in
a bundled library for a single method.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144685933
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
> ... any reason not to use Apache Commons IO?
If you want to use Apache Commons IO, I'm not against it.
To me, the only reason would be to avoid bundling a library from which
we're only going to use one function. We've removed dependencies in the past
for similar reasons (see #37), but in that case there was a reasonable
alternative already available within Java itself. If the alternative in the
case of #37 was to write our own implementation entirely, I'm sure we would
have kept the dependency around.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144681030
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
Well, I think I have a reasonable ByteArrayOutputStream solution, but,
before I do that, any reason not to use Apache Commons IO?
https://commons.apache.org/proper/commons-io/javadocs/api-2.5/org/apache/commons/io/FileUtils.html#readFileToByteArray(java.io.File)
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144460945
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
Looks better, however:
1. The duplicate `read()` calls are somewhat odd.
2. This loop implementation will spin indefinitely. When the buffer is
filled, `keyBytes.length - totalBytesRead` becomes 0, in which case the call to
`read()` will be asked to read 0 bytes and will always return 0, never
signalling EOF.
It might be a lot easier to implement correctly if the byte array is
allocated dynamically. That would also allow for cases where the file size is
not known ahead of time, and remove the need to handle unexpected changes in
file size.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143899357
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
Okay, took a stab at a loop implementation of this...
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890955
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
+int keyRead = keyInput.read(keyBytes);
+
+// Error reading any bytes out of the key.
+if (keyRead == -1)
--- End diff --
Normally, this is the condition you would check to determine whether to
stop reading the file due to reaching EOF.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890795
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
+int keyRead = keyInput.read(keyBytes);
+
+// Error reading any bytes out of the key.
+if (keyRead == -1)
+throw new GuacamoleServerException("Failed to get any
bytes while reading key.");
+
+// Zero-sized key
+else if(keyRead == 0)
+throw new GuacamoleServerException("Failed to ready key
because key is empty.");
+
+// Fewer bytes read than contained in the key
+else if (keyRead < keyLength)
--- End diff --
When reading in data from a file, while it's not guaranteed that each read
will fill the buffer, returning fewer bytes than desired does not mean that
future reads will fail. As long as end-of-file hasn't been reached (`read()`
returns -1), and no error has been indicated (`IOException`), then `read()`
should continue to be called until the buffer has been filled.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890893
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
+int keyRead = keyInput.read(keyBytes);
+
+// Error reading any bytes out of the key.
+if (keyRead == -1)
+throw new GuacamoleServerException("Failed to get any
bytes while reading key.");
+
+// Zero-sized key
+else if(keyRead == 0)
--- End diff --
A `read()` of 0 does not indicate that there are zero bytes in the file,
nor any error condition. It simply means that `read()` read nothing, and should
be invoked again.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890461
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -36,48 +47,134 @@
public class TicketValidationService {
/**
+ * Logger for this class.
+ */
+private static final Logger logger =
LoggerFactory.getLogger(TicketValidationService.class);
+
+/**
* Service for retrieving CAS configuration information.
*/
@Inject
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the username
contained
- * therein, as defined by the username claim type given in
- * guacamole.properties. If the username claim type is missing or the
ID
- * ticket is invalid, an exception is thrown instead.
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
+ * ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
*
+ * @param credentials
+ * The Credentials object to store retrieved username and
+ * password values in.
+ *
* @return
- * The username contained within the given ID ticket.
+ * The username derived from the ticket.
*
* @throws GuacamoleException
- * If the ID ticket is not valid, the username claim type is
missing, or
- * guacamole.properties could not be parsed.
+ * If the ID ticket is not valid or guacamole.properties could
+ * not be parsed.
*/
-public String processUsername(String ticket) throws GuacamoleException
{
-
-AttributePrincipal principal = null;
+public String validateTicket(String ticket, Credentials credentials)
throws GuacamoleException {
// Retrieve the configured CAS URL, establish a ticket validator,
// and then attempt to validate the supplied ticket. If that
succeeds,
// grab the principal returned by the validator.
String casServerUrl = confService.getAuthorizationEndpoint();
Cas20ProxyTicketValidator validator = new
Cas20ProxyTicketValidator(casServerUrl);
validator.setAcceptAnyProxy(true);
+validator.setEncoding("UTF-8");
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
-principal = a.getPrincipal();
+AttributePrincipal principal = a.getPrincipal();
+
+// Retrieve username and set the credentials.
+String username = principal.getName();
+if (username != null)
+credentials.setUsername(username);
+
+// Retrieve password, attempt decryption, and set credentials.
+Object credObj = principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
+if (clearPass != null && !clearPass.isEmpty())
+credentials.setPassword(clearPass);
+}
+
+return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
-// Return the principal name as the username.
-return principal.getName();
+}
+
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143891620
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+int keyLength = (int) keyFile.length();
+final byte[] keyBytes = new byte[keyLength];
--- End diff --
Beware that if you pre-allocate the `byte[]` like this, we'll need to bail
out if the file size differs from what is expected. I don't think that's bad
necessarily, so long as the case is handled.
The alternative would be to dynamically allocate the `byte[]`. An easy way
would be to use something like a
[`ByteArrayOutputStream`](https://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayOutputStream.html),
reading/writing chunks in a loop.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142038882
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws
GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
-principal = a.getPrincipal();
+AttributePrincipal principal = a.getPrincipal();
+
+// Retrieve username and set the credentials.
+String username = principal.getName();
+if (username != null)
+credentials.setUsername(username);
+
+// Retrieve password, attempt decryption, and set credentials.
+Object credObj = principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
+if (clearPass != null && !clearPass.isEmpty())
+credentials.setPassword(clearPass);
+}
+
+return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
-// Return the principal name as the username.
-return principal.getName();
+}
+
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
--- End diff --
I changed the warn to debug. My thought process is that, if you are
setting up both CAS and Guacamole and you're looking for hints as to why it
isn't working, you want a message that indicates that the private key isn't
loading. I don't know that the order of these first two statements makes all
that much difference - I can have the check for private key come, first, if
that's more desirable, but either way it needs to check for both the presence
of the encrypted password and the private key before continuing.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142033722
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws
GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
-principal = a.getPrincipal();
+AttributePrincipal principal = a.getPrincipal();
+
+// Retrieve username and set the credentials.
+String username = principal.getName();
+if (username != null)
+credentials.setUsername(username);
+
+// Retrieve password, attempt decryption, and set credentials.
+Object credObj = principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
+if (clearPass != null && !clearPass.isEmpty())
+credentials.setPassword(clearPass);
+}
+
+return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
-// Return the principal name as the username.
-return principal.getName();
+}
+
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
--- End diff --
Right, I don't think authentication should fail in that case, but it seems
odd that the behavior for a server with this feature enabled is to warn "No
private key available to decrypt password" for every successful login.
What I'm aiming at is: shouldn't this functionality only be active if the
key was provided in the first place?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142027025
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws
GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
-principal = a.getPrincipal();
+AttributePrincipal principal = a.getPrincipal();
+
+// Retrieve username and set the credentials.
+String username = principal.getName();
+if (username != null)
+credentials.setUsername(username);
+
+// Retrieve password, attempt decryption, and set credentials.
+Object credObj = principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
+if (clearPass != null && !clearPass.isEmpty())
+credentials.setPassword(clearPass);
+}
+
+return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
-// Return the principal name as the username.
-return principal.getName();
+}
+
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
+return null;
+}
+
+try {
+
+final Cipher cipher =
Cipher.getInstance(clearpassKey.getAlgorithm());
+
+if (cipher == null)
+throw new GuacamoleServerException("Failed to initialize
cipher object with private key.");
+
+// Initialize the Cipher in decrypt mode.
+cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+// Decode and decrypt, and return a new string.
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
--- End diff --
I'll have to dig into the CAS side and see.
Is there a preferred way to convert byte[] data to a String in Java that is
considered production-safe?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142027015
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws
GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
-principal = a.getPrincipal();
+AttributePrincipal principal = a.getPrincipal();
+
+// Retrieve username and set the credentials.
+String username = principal.getName();
+if (username != null)
+credentials.setUsername(username);
+
+// Retrieve password, attempt decryption, and set credentials.
+Object credObj = principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
+if (clearPass != null && !clearPass.isEmpty())
+credentials.setPassword(clearPass);
+}
+
+return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
-// Return the principal name as the username.
-return principal.getName();
+}
+
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
--- End diff --
Yes, that is desired - since there are situations where it is conceivable
that the Guacamole administrator does not have control over the CAS server and
what attributes are returned by the server, it would be undesirable to fail
authentication completely just because CAS returned attributes that Guacamole
was not expecting. While that's unlikely in the case of ClearPass, I still
believe it is the correct behavior.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019920
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws
GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
-principal = a.getPrincipal();
+AttributePrincipal principal = a.getPrincipal();
+
+// Retrieve username and set the credentials.
+String username = principal.getName();
+if (username != null)
+credentials.setUsername(username);
+
+// Retrieve password, attempt decryption, and set credentials.
+Object credObj = principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
+if (clearPass != null && !clearPass.isEmpty())
+credentials.setPassword(clearPass);
+}
+
+return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
-// Return the principal name as the username.
-return principal.getName();
+}
+
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
--- End diff --
This will result in warnings for any system where CAS is configured to
return credentials, but Guacamole is not configured to use those returned
credentials. Is that the desired behavior?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019875
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws
GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
-principal = a.getPrincipal();
+AttributePrincipal principal = a.getPrincipal();
+
+// Retrieve username and set the credentials.
+String username = principal.getName();
+if (username != null)
+credentials.setUsername(username);
+
+// Retrieve password, attempt decryption, and set credentials.
+Object credObj = principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
+if (clearPass != null && !clearPass.isEmpty())
+credentials.setPassword(clearPass);
+}
+
+return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
-// Return the principal name as the username.
-return principal.getName();
+}
+
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
+return null;
+}
+
+try {
+
+final Cipher cipher =
Cipher.getInstance(clearpassKey.getAlgorithm());
+
+if (cipher == null)
+throw new GuacamoleServerException("Failed to initialize
cipher object with private key.");
+
+// Initialize the Cipher in decrypt mode.
+cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+// Decode and decrypt, and return a new string.
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
--- End diff --
The `String` constructor which takes only a `byte[]` uses the default
charset, and is generally considered unsafe for production code. The encoding
used by the data within the `byte[]` should be known.
Does CAS define this?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019841
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+return keyFactory.generatePrivate(keySpec);
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleServerException("Could not find the
specified key file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("Could not read in the
specified key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleServerException("Key is not in expected RSA
algorithm.", e);
--- End diff --
The `NoSuchAlgorithmException` does not indicate that the key is not a
valid RSA key, but rather that the RSA algorithm isn't available (ie: the JVM
in use doesn't provide it).
https://docs.oracle.com/javase/8/docs/api/java/security/NoSuchAlgorithmException.html
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019823
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
--- End diff --
`read()` isn't guaranteed to read in 100% of the bytes requested, nor is it
guaranteed to succeed. The return value of `read()` will need to be handled if
you wish to reliably read the key file into the byte array.
See:
https://docs.oracle.com/javase/7/docs/api/java/io/InputStream.html#read()
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019777
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+return keyFactory.generatePrivate(keySpec);
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleServerException("Could not find the
specified key file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("Could not read in the
specified key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleServerException("Key is not in expected RSA
algorithm.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleServerException("KeyS is not in expected
PKCS8 encoding.", e);
--- End diff --
"KeyS"
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019767
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -36,30 +46,35 @@
public class TicketValidationService {
/**
+ * Logger for this class.
+ */
+private static final Logger logger =
LoggerFactory.getLogger(TicketValidationService.class);
+
+/**
* Service for retrieving CAS configuration information.
*/
@Inject
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the username
contained
- * therein, as defined by the username claim type given in
- * guacamole.properties. If the username claim type is missing or the
ID
- * ticket is invalid, an exception is thrown instead.
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
+ * ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
+ * @param credentials
--- End diff --
Mind adding the blank line above `@param credentials` to match the style of
other comments?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019203
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -57,21 +57,21 @@
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the Credentials
object
- * derived from the parameters provided by the CAS server in the
ticket. If the
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
* ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
*
* @return
- * The Credentials object derived from parameters provided in the
ticket.
+ * The username derived from the ticket.
*
* @throws GuacamoleException
* If the ID ticket is not valid or guacamole.properties could
* not be parsed.
*/
-public Credentials validateTicket(String ticket) throws
GuacamoleException {
+public String validateTicket(String ticket, Credentials credentials)
throws GuacamoleException {
--- End diff --
Oops...added.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019015
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -57,21 +57,21 @@
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the Credentials
object
- * derived from the parameters provided by the CAS server in the
ticket. If the
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
* ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
*
* @return
- * The Credentials object derived from parameters provided in the
ticket.
+ * The username derived from the ticket.
*
* @throws GuacamoleException
* If the ID ticket is not valid or guacamole.properties could
* not be parsed.
*/
-public Credentials validateTicket(String ticket) throws
GuacamoleException {
+public String validateTicket(String ticket, Credentials credentials)
throws GuacamoleException {
--- End diff --
The new `credentials` parameter needs to be documented, too.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142018263
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -82,8 +87,17 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
if (request != null) {
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
+Credentials ticketCredentials =
ticketService.validateTicket(ticket);
+if (ticketCredentials != null) {
+String username = ticketCredentials.getUsername();
+if (username != null)
+credentials.setUsername(username);
+String password = ticketCredentials.getPassword();
+if (password != null)
+credentials.setPassword(password);
+}
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+authenticatedUser.init(credentials.getUsername(),
credentials);
--- End diff --
Okay, I refactored as you suggested, and I believe I have all of the
username objects coming from CAS.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141926972
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -82,8 +87,17 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
if (request != null) {
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
+Credentials ticketCredentials =
ticketService.validateTicket(ticket);
+if (ticketCredentials != null) {
+String username = ticketCredentials.getUsername();
+if (username != null)
+credentials.setUsername(username);
+String password = ticketCredentials.getPassword();
+if (password != null)
+credentials.setPassword(password);
+}
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+authenticatedUser.init(credentials.getUsername(),
credentials);
--- End diff --
This is dangerous. Consider the following flow:
1. A malicious user sets the `username` query parameter to a known user
(say, "guacadmin"), knowing that Guacamole will automatically populate the
`Credentials` object with that value.
2. `ticketService.validateTicket()` does not set the username due to
`principal.getName()` returning `null`, thus the username of `credentials`
remains unchanged.
3. After CAS authentication completes, the user is given the identity of
the user they specified, rather than the identity verified by CAS.
The identity of the user should come purely from CAS.
I would suggest:
* Passing the main `Credentials` object into `validateTicket()`, relying on
`validateTicket()` to handle assignment of username/password values.
* Going back to simply returning the username, rather than a `Credentials`
object which must be manually copied into the main `Credentials` object,
trusting *only* the value which comes from CAS.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141375864
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -83,7 +115,15 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+AttributePrincipal principal =
ticketService.validateTicket(ticket);
+String username = principal.getName();
+Object credObj =
principal.getAttributes().get("credential");
+if (credObj != null) {
--- End diff --
Okay, I've moved a bunch of the decryption logic over to the
TicketValidationService class and am returning a CredentialsObject back to the
AuthenticationProviderService, instead. How does that look?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141365440
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
+return null;
+}
+
+try {
+
+final Cipher cipher =
Cipher.getInstance(clearpassKey.getAlgorithm());
+
+if (cipher == null)
+throw new GuacamoleServerException("Failed to initialize
cipher object with private key.");
+
+// Initialize the Cipher in decrypt mode.
+cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+// Decode and decrypt, and return a new string.
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
+
+}
+catch (Throwable t) {
--- End diff --
Should be good - let me know if any of the error messages need tweaking.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141360655
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141361436
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -83,7 +115,15 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+AttributePrincipal principal =
ticketService.validateTicket(ticket);
+String username = principal.getName();
+Object credObj =
principal.getAttributes().get("credential");
+if (credObj != null) {
--- End diff --
Let me see what I can figure out. That's essentially what it was before,
and only the username was returned. When this transitioned to also providing a
password I re-arranged things so that it would be a little more
straight-forward to retrieve both the username and the password from the
AttributePrincipal object without having to try to pass a HashMap or something
like that. If you have a direction in mind let me know...otherwise I'll just
take a stab at something and see what I can figure out.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141360602
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+return keyFactory.generatePrivate(keySpec);
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleServerException("Could not find the
specified key file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("Could not read in the
specified key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleServerException("Specified algorithm does
not exist.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleServerException("Invalid KeySpec
initialization.", e);
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141361788
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
+return null;
+}
+
+try {
+
+final Cipher cipher =
Cipher.getInstance(clearpassKey.getAlgorithm());
+
+if (cipher == null)
+throw new GuacamoleServerException("Failed to initialize
cipher object with private key.");
+
+// Initialize the Cipher in decrypt mode.
+cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+// Decode and decrypt, and return a new string.
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
+
+}
+catch (Throwable t) {
--- End diff --
Okay, I'll go back to catching them each individually and go that route.
Thanks.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141359733
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -57,9 +57,7 @@
* If the ID ticket is not valid, the username claim type is
missing, or
* guacamole.properties could not be parsed.
*/
-public String processUsername(String ticket) throws GuacamoleException
{
-
-AttributePrincipal principal = null;
+public AttributePrincipal validateTicket(String ticket) throws
GuacamoleException {
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141360569
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+return keyFactory.generatePrivate(keySpec);
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleServerException("Could not find the
specified key file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("Could not read in the
specified key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleServerException("Specified algorithm does
not exist.", e);
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141267616
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
+return null;
+}
+
+try {
+
+final Cipher cipher =
Cipher.getInstance(clearpassKey.getAlgorithm());
+
+if (cipher == null)
+throw new GuacamoleServerException("Failed to initialize
cipher object with private key.");
+
+// Initialize the Cipher in decrypt mode.
+cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+// Decode and decrypt, and return a new string.
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
+
+}
+catch (Throwable t) {
--- End diff --
Yeah, catching `Throwable` is generally bad. There are cases where it's
necessary, but they're not common. Best practice is normally to explicitly
catch the exceptions which apply, catching superclasses of those exceptions
where it makes sense to do so.
It doesn't have to be pointless - if the context of each relevant exception
is known, you can include a meaningful message in the
`GuacamoleServerException` which you use to wrap the caught exception, rather
than simply rethrowing things.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266516
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
--- End diff --
This function returns a `PrivateKey`, not a path.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266837
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -83,7 +115,15 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+AttributePrincipal principal =
ticketService.validateTicket(ticket);
+String username = principal.getName();
+Object credObj =
principal.getAttributes().get("credential");
+if (credObj != null) {
--- End diff --
The complexity of what's going on here is increasing significantly.
Thoughts on encapsulating this credential retrieval logic within the ticket
service, for maintainability/readability's sake?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266397
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+return keyFactory.generatePrivate(keySpec);
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleServerException("Could not find the
specified key file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("Could not read in the
specified key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleServerException("Specified algorithm does
not exist.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleServerException("Invalid KeySpec
initialization.", e);
--- End diff --
Is there a more meaningful error message for the failure which results in
this exception?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266280
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java
---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public PrivateKey parseValue(String value) throws
GuacamoleServerException {
+
+if (value == null || value.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+return keyFactory.generatePrivate(keySpec);
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleServerException("Could not find the
specified key file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleServerException("Could not read in the
specified key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleServerException("Specified algorithm does
not exist.", e);
--- End diff --
Since we're the ones specifying the algorithm, we should probably
specifically state the cause of the error in this case, as it is known.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141265939
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -57,9 +57,7 @@
* If the ID ticket is not valid, the username claim type is
missing, or
* guacamole.properties could not be parsed.
*/
-public String processUsername(String ticket) throws GuacamoleException
{
-
-AttributePrincipal principal = null;
+public AttributePrincipal validateTicket(String ticket) throws
GuacamoleException {
--- End diff --
The documentation for this function states:
> Validates and parses the given ID ticket, returning the username
contained therein, ...
This was from when the function returned a `String` and was called
`processUsername()`. Now that the function has been reworked to return an
`AttributePrincipal` (and renamed to `validateTicket()`), is this documentation
still valid?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140944932
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
+return null;
+}
+
+try {
+
+final Cipher cipher =
Cipher.getInstance(clearpassKey.getAlgorithm());
+
+if (cipher == null)
+throw new GuacamoleServerException("Failed to initialize
cipher object with private key.");
+
+// Initialize the Cipher in decrypt mode.
+cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+// Decode and decrypt, and return a new string.
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
+
+}
+catch (Throwable t) {
--- End diff --
I think the number of things I was catching individually was getting really
long - after I hit 8, I think, I decided maybe just catching Throwable was
easier than going through and catching them each individually.
Is catching Throwable considered poor form (lazy, perhaps :-) vs. catching
each item individually, particularly if they're all going to be re-thrown as
GuacamoleServerExceptions, anyway? I'll go back the other way if that's the
proper way to do it...
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140689214
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+logger.warn("No or empty encrypted password, no password will
be available.");
+return null;
+}
+
+final PrivateKey clearpassKey = confService.getClearpassKey();
+if (clearpassKey == null) {
+logger.warn("No private key available to decrypt password.");
+return null;
+}
+
+try {
+
+final Cipher cipher =
Cipher.getInstance(clearpassKey.getAlgorithm());
+
+if (cipher == null)
+throw new GuacamoleServerException("Failed to initialize
cipher object with private key.");
+
+// Initialize the Cipher in decrypt mode.
+cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+// Decode and decrypt, and return a new string.
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
+
+}
+catch (Throwable t) {
--- End diff --
Why are we catching `Throwable`?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140666358
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
--- End diff --
Pulled out home env call and we'll just require absolute path.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140666346
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(environment.getGuacamoleHome(), value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+return cipher;
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleException("Could not find the specified key
file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleException("Could not read in the specified
key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleException("Specified algorithm does not
exist.", e);
+}
+catch (InvalidKeyException e) {
+throw new GuacamoleException("Specified key is invalid.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleException("Invalid KeySpec
initialization.", e);
+}
+catch (NoSuchPaddingException e) {
+throw new GuacamoleException("No such padding exception.", e);
+}
+
--- End diff --
So, I've redone most of this such that it throws the
GuacamoleServerException. There are two scenarios I can think of where having
authentication succeed despite some error in the ClearPass decryption process
would be desirable:
- If the credentials object is provided by the CAS server, but the
Guacamole admin has not configured a private key, I think authentication should
still succeed. Since, in many organizations, SSO is run by someone different
than a VDI/Desktop/RemoteAccess person, it's conceivable that the CAS server
may provide something we choose not to consume, and that should not cause an
error.
- Where the Guacamole admin has configured a PrivateKey, but CAS
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140666133
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+public Cipher getClearpassCipher() throws GuacamoleException {
+return
environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+}
+
--- End diff --
Refactored as PrivateKey.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140645414
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
--- End diff --
Absolute path is fine - I think there are a few other similar properties
(basic user mapping, even) that do that, so it isn't without precedent. I
guess it just seems like assuming it'll be in GUACAMOLE_HOME feels a little
cleaner from a configuration perspective - admins don't have to keep guessing
which properties assume absolutely paths and which are relative to
GUACAMOLE_HOME - but as long as it's properly documented it should be okay.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140645395
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(environment.getGuacamoleHome(), value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+return cipher;
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleException("Could not find the specified key
file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleException("Could not read in the specified
key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleException("Specified algorithm does not
exist.", e);
+}
+catch (InvalidKeyException e) {
+throw new GuacamoleException("Specified key is invalid.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleException("Invalid KeySpec
initialization.", e);
+}
+catch (NoSuchPaddingException e) {
+throw new GuacamoleException("No such padding exception.", e);
+}
+
--- End diff --
> Is it expected that this will occasionally fail under normal (not
misconfigured) conditions?
The more I think about it, the more I think this probably would not fail
under anything but a misconfiguration. In order to get ClearPass to work in
CAS you have to generate public and private keys, and configure a service in
CAS specifically to support the encryption of the stored password, so anyone
who is setting this up also has to be setting up the CAS side of things, and if
they get it wrong, well, it probably shouldn't work at all. I'll change the
exception to GuacamoleServerException and stick with failing authentication if
this fails.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140645375
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+public Cipher getClearpassCipher() throws GuacamoleException {
+return
environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+}
+
--- End diff --
Yeah, PrivateKey is probably a better route to go, and just keep the Cipher
pieces of it in the CAS extension code. I'll try to rework this a little bit.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140011656
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(environment.getGuacamoleHome(), value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+return cipher;
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleException("Could not find the specified key
file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleException("Could not read in the specified
key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleException("Specified algorithm does not
exist.", e);
+}
+catch (InvalidKeyException e) {
+throw new GuacamoleException("Specified key is invalid.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleException("Invalid KeySpec
initialization.", e);
+}
+catch (NoSuchPaddingException e) {
+throw new GuacamoleException("No such padding exception.", e);
+}
+
--- End diff --
> ... so instead of returning null, here, I'm throwing GuacamoleException.
Sounds good. I'm always in favor of strict error handling, though
[`GuacamoleServerException`](http://guacamole.incubator.apache.org/doc/guacamole-common/org/apache/guacamole/GuacamoleServerException.html)
may be a better choice.
> This means that, if those exceptions are thrown while CAS is using this
property, it'll kill CAS authentication if this Cipher property fails to parse
for one of these reasons. I consider that behavior less desirable than just not
having the password available to the user, as authentication has technically
still succeeded.
Is it expected that this will o
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r139979934
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
--- End diff --
Hm ... I see what you mean.
It would be possible to get Guice to inject the `Environment` by using a
`Provider`, thus avoiding recreating things, but
perhaps there is a different way.
This may end up reverting things back to how you had them, but what about
relying on the value of this property to be the absolute path to the key? If
the property itself ends up needing to be simply a `File` or `String`, with the
actual reading into `PrivateKey` and finally `Cipher` being abstracted only
within the configuration service, that's fine, I'm sure.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r139978510
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+public Cipher getClearpassCipher() throws GuacamoleException {
+return
environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+}
+
--- End diff --
I would tend to agree there. Why not `PrivateKey`?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135947971
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(environment.getGuacamoleHome(), value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+return cipher;
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleException("Could not find the specified key
file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleException("Could not read in the specified
key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleException("Specified algorithm does not
exist.", e);
+}
+catch (InvalidKeyException e) {
+throw new GuacamoleException("Specified key is invalid.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleException("Invalid KeySpec
initialization.", e);
+}
+catch (NoSuchPaddingException e) {
+throw new GuacamoleException("No such padding exception.", e);
+}
+
--- End diff --
When all of those exceptions were caught and dealt with directly in the
AuthenticationProviderService class, the mehtod just returned null rather than
throwing a GuacamoleException. Returning null just means that the password is
not added to the credential object, so it isn't available as a token later on
inside the client.
Since creating a more generic type, here, I figured, while the behavior of
just returning a null value and not killing authentication entirely is fine for
the CAS extension, it may not be desirable across the board for whatever stuff
may come later that might use this property, so instead of returning null,
here, I'm throwing GuacamoleException. This
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135920241
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(environment.getGuacamoleHome(), value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+return cipher;
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleException("Could not find the specified key
file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleException("Could not read in the specified
key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleException("Specified algorithm does not
exist.", e);
+}
+catch (InvalidKeyException e) {
+throw new GuacamoleException("Specified key is invalid.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleException("Invalid KeySpec
initialization.", e);
+}
+catch (NoSuchPaddingException e) {
+throw new GuacamoleException("No such padding exception.", e);
+}
+
--- End diff --
What do you mean by "killed that off"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426202
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
--- End diff --
This feels like reverting back to behavior advised against before, but I'm
not sure the best way to get the Guacamole Home directory into this property to
open up the file without doing this. Suggestions?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426332
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+public Cipher getClearpassCipher() throws GuacamoleException {
+return
environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+}
+
--- End diff --
Taking a key argument and calling this a cipher property feels a little
kludgy. Kind of goes along with the question about how far the Cipher property
should go or if that's even really a good name/function for it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426151
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(environment.getGuacamoleHome(), value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
--- End diff --
So, I'm not completely sure that this is the cleanest way to do this, or
what the boundary of this property should be. I implemented it as a Cipher
property, which takes in the filename of a private key relative to the
guacamole home directory and generates the Cipher object out of it necessary to
decrypt some string text. Thoughts on whether this is the correct level to go
to, or if I should go higher-level or lower-level?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426231
--- Diff:
guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java
---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements
GuacamoleProperty {
+
+@Override
+public Cipher parseValue(String value) throws GuacamoleException {
+
+try {
+
+final Environment environment = new LocalEnvironment();
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(environment.getGuacamoleHome(), value);
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+return cipher;
+
+}
+catch (FileNotFoundException e) {
+throw new GuacamoleException("Could not find the specified key
file.", e);
+}
+catch (IOException e) {
+throw new GuacamoleException("Could not read in the specified
key file.", e);
+}
+catch (NoSuchAlgorithmException e) {
+throw new GuacamoleException("Specified algorithm does not
exist.", e);
+}
+catch (InvalidKeyException e) {
+throw new GuacamoleException("Specified key is invalid.", e);
+}
+catch (InvalidKeySpecException e) {
+throw new GuacamoleException("Invalid KeySpec
initialization.", e);
+}
+catch (NoSuchPaddingException e) {
+throw new GuacamoleException("No such padding exception.", e);
+}
+
--- End diff --
And, you know how I said that the exceptions shouldn't kill the entire
client, it should just result in the password not being available...well...I
think all of this here killed that off...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134774695
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +69,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+public File getClearpassKey() throws GuacamoleException {
--- End diff --
Oops...I was thinking the wrong way, here. So, it wouldn't be
EncryptedGuacamoleProperty, it would be something like SSLGuacamoleProperty or
PrivateKeyGuacamoleProperty or something like that...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134767849
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(new
LocalEnvironment().getGuacamoleHome(),
confService.getClearpassKey().toString());
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+// Decrypt and return a new string.
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
+}
+catch (FileNotFoundException e) {
+logger.error("ClearPass key file not found, password will not
be decrypted.");
+logger.debug("Error locating the ClearPass key file: {}",
e.getMessage());
--- End diff --
This should be fixed - I don't know that any of the error messages are
worth passing to logger.error(), so I just those as-is, and changed
logger.debug() to include the full exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134767686
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(new
LocalEnvironment().getGuacamoleHome(),
confService.getClearpassKey().toString());
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
--- End diff --
Added this exception to the list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134765996 --- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java --- @@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials) } +/** + * Takes an encrypted string representing a password provided by + * the CAS ClearPass service and decrypts it using the private + * key configured for this extension. Returns null if it is + * unable to decrypt the password. + * + * @param encryptedPassword + * A string with the encrypted password provided by the + * CAS service. + * + * @return + * The decrypted password, or null if it is unable to + * decrypt the password. + * @throws GuacamoleException --- End diff -- Fixed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at [email protected] or file a JIRA ticket with INFRA. ---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134765905
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(new
LocalEnvironment().getGuacamoleHome(),
confService.getClearpassKey().toString());
--- End diff --
Should be fixed, though it won't matter if this gets pulled out into its
own property type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134763447
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -83,7 +106,16 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+AttributePrincipal principal =
ticketService.validateTicket(ticket);
+String username = principal.getName();
+credentials.setUsername(username);
+Object credObj =
principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
--- End diff --
The getAttributes call returns a Map, so I think this
conversion is the only (safe) way to get this object into the string format
needed for the decryption process.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134762077 --- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java --- @@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials) } +/** + * Takes an encrypted string representing a password provided by + * the CAS ClearPass service and decrypts it using the private + * key configured for this extension. Returns null if it is + * unable to decrypt the password. + * + * @param encryptedPassword + * A string with the encrypted password provided by the + * CAS service. + * + * @return + * The decrypted password, or null if it is unable to + * decrypt the password. + * @throws GuacamoleException + * If unable to get Guacamole configuration data --- End diff -- My rationale here was that it's actually okay, and should not halt the login process, if either the credential object is not available, or it can't be decrypted. Failure to retrieve or decrypt this property, IMHO, should not stop the login process from working or connections from being made - the token simply won't be available. Is there an exception that should be thrown that would not cause the login process to stop? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at [email protected] or file a JIRA ticket with INFRA. ---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134761374
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +69,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+public File getClearpassKey() throws GuacamoleException {
--- End diff --
That's probably reasonable - I'll work on moving this over to some sort of
EncryptedGuacamoleProperty or something like that. Preferences on naming??
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134754446
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
---
@@ -57,9 +57,7 @@
* If the ID ticket is not valid, the username claim type is
missing, or
* guacamole.properties could not be parsed.
*/
-public String processUsername(String ticket) throws GuacamoleException
{
-
-AttributePrincipal principal = null;
--- End diff --
Glad to see this line go. ;) Never a fan of unnecessary initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134759742
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -83,7 +106,16 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
String ticket =
request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
-
authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+AttributePrincipal principal =
ticketService.validateTicket(ticket);
+String username = principal.getName();
+credentials.setUsername(username);
+Object credObj =
principal.getAttributes().get("credential");
+if (credObj != null) {
+String clearPass = decryptPassword(credObj.toString());
--- End diff --
Is there a defined object type for what `get("credential")` returns? Or is
conversion to a string and back again the only way?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134759279
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(new
LocalEnvironment().getGuacamoleHome(),
confService.getClearpassKey().toString());
--- End diff --
Is an `Environment` instance already created and available? Rather than
creating a new `LocalEnvironment` each time an attempt to decrypt the password
is made, it would be better to share the existing `Environment` used by the
rest of the extension.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134758113
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(new
LocalEnvironment().getGuacamoleHome(),
confService.getClearpassKey().toString());
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
+cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+// Decrypt and return a new string.
+final byte[] cipherData = cipher.doFinal(pass64);
+return new String(cipherData);
+}
+catch (FileNotFoundException e) {
+logger.error("ClearPass key file not found, password will not
be decrypted.");
+logger.debug("Error locating the ClearPass key file: {}",
e.getMessage());
--- End diff --
`logger.debug()` calls should receive the exception itself, not just the
message. That way, when debug logging is enabled, full stacktraces are logged.
Assuming the exception message is meaningful, it's the higher-level log
statement which would receive `e.getMessage()` rather than `e` (in this case,
the call to `logger.error()`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134756239
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials
credentials)
}
+/**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+private final String decryptPassword(String encryptedPassword)
+throws GuacamoleException {
+
+// If we get nothing, we return nothing.
+if (encryptedPassword == null || encryptedPassword.isEmpty())
+return null;
+
+try {
+
+// Open and read the file specified in the configuration.
+File keyFile = new File(new
LocalEnvironment().getGuacamoleHome(),
confService.getClearpassKey().toString());
+InputStream keyInput = new BufferedInputStream(new
FileInputStream(keyFile));
+final byte[] keyBytes = new byte[(int) keyFile.length()];
+keyInput.read(keyBytes);
+keyInput.close();
+
+// Set up decryption infrastructure
+KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+final PrivateKey privateKey =
keyFactory.generatePrivate(keySpec);
+final Cipher cipher =
Cipher.getInstance(privateKey.getAlgorithm());
+final byte[] pass64 =
DatatypeConverter.parseBase64Binary(encryptedPassword);
--- End diff --
Beware that this will throw an `IllegalArgumentException` provided data is
not valid base64. That case will need to be handled for this function to behave
in a robust manner.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134755694
--- Diff:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
---
@@ -68,4 +69,20 @@ public String getRedirectURI() throws GuacamoleException
{
return
environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+/**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+public File getClearpassKey() throws GuacamoleException {
--- End diff --
Thoughts on implementing a `GuacamoleProperty` subclass which handles the
parsing of the key value, thus abstracting away the reading of its contents as
`KeySpec` or `PrivateKey` or similar?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request: https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134758567 --- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java --- @@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials) } +/** + * Takes an encrypted string representing a password provided by + * the CAS ClearPass service and decrypts it using the private + * key configured for this extension. Returns null if it is + * unable to decrypt the password. + * + * @param encryptedPassword + * A string with the encrypted password provided by the + * CAS service. + * + * @return + * The decrypted password, or null if it is unable to + * decrypt the password. + * @throws GuacamoleException --- End diff -- Missing blank line separating the `@return` block from the `@throws`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at [email protected] or file a JIRA ticket with INFRA. ---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Github user mike-jumper commented on a diff in the pull request: https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134757155 --- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java --- @@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials) } +/** + * Takes an encrypted string representing a password provided by + * the CAS ClearPass service and decrypts it using the private + * key configured for this extension. Returns null if it is + * unable to decrypt the password. + * + * @param encryptedPassword + * A string with the encrypted password provided by the + * CAS service. + * + * @return + * The decrypted password, or null if it is unable to + * decrypt the password. + * @throws GuacamoleException + * If unable to get Guacamole configuration data --- End diff -- All exceptions are actually caught and masked by that function, with any failures whatsoever resulting in return of `null`. Why not rethrow those failures as `GuacamoleServerException` or similar? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at [email protected] or file a JIRA ticket with INFRA. ---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
GitHub user necouchman opened a pull request: https://github.com/apache/incubator-guacamole-client/pull/183 GUACAMOLE-362: Add support for CAS ClearPass This PR adds support to the CAS module for the ClearPass functionality, which allows CAS to pass the password back to the requesting service in an encrypted fashion, and the service will decrypt the password. The password gets assigned to the credentials for the module, so it can be used as a token. As a side-effect, this also resolves GUACAMOLE-341 for the CAS module, which deals we the username being available in the credentials object for SSO authentication extensions. You can merge this pull request into a Git repository by running: $ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-362 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-guacamole-client/pull/183.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #183 commit a7effd027b87f7f0154f51c3a4afed33c8d1cc8e Author: Nick Couchman Date: 2017-08-16T15:58:26Z GUACAMOLE-362: Add support for CAS ClearPass, parsing and decrypting the password and assigning it a token. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at [email protected] or file a JIRA ticket with INFRA. ---
