Skip to content

CLOUDSTACK-9593: userdata: enforce data is a multiple of 4 characters #1760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yadvr merged 1 commit into from
Dec 28, 2017
Merged

CLOUDSTACK-9593: userdata: enforce data is a multiple of 4 characters #1760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions engine/schema/src/com/cloud/upgrade/dao/Upgrade41000to41100.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.Map;
import java.util.Set;

import org.apache.commons.codec.binary.Base64;
import org.apache.log4j.Logger;

import com.cloud.hypervisor.Hypervisor;
Expand Down Expand Up @@ -64,9 +65,49 @@ public InputStream[] getPrepareScripts() {

@Override
public void performDataMigration(Connection conn) {
validateUserDataInBase64(conn);
updateSystemVmTemplates(conn);
}

private void validateUserDataInBase64(Connection conn) {
try (final PreparedStatement selectStatement = conn.prepareStatement("SELECT `id`, `user_data` FROM `cloud`.`user_vm` WHERE `user_data` IS NOT NULL;");
final ResultSet selectResultSet = selectStatement.executeQuery()) {
while (selectResultSet.next()) {
final Long userVmId = selectResultSet.getLong(1);
final String userData = selectResultSet.getString(2);
if (Base64.isBase64(userData)) {
final String newUserData = Base64.encodeBase64String(Base64.decodeBase64(userData.getBytes()));
if (!userData.equals(newUserData)) {
try (final PreparedStatement updateStatement = conn.prepareStatement("UPDATE `cloud`.`user_vm` SET `user_data` = ? WHERE `id` = ? ;")) {
updateStatement.setString(1, newUserData);
updateStatement.setLong(2, userVmId);
updateStatement.executeUpdate();
} catch (SQLException e) {
LOG.error("Failed to update cloud.user_vm user_data for id:" + userVmId + " with exception: " + e.getMessage());
throw new CloudRuntimeException("Exception while updating cloud.user_vm for id " + userVmId, e);
}
}
} else {
// Update to NULL since it's invalid
LOG.warn("Removing user_data for vm id " + userVmId + " because it's invalid");
LOG.warn("Removed data was: " + userData);
try (final PreparedStatement updateStatement = conn.prepareStatement("UPDATE `cloud`.`user_vm` SET `user_data` = NULL WHERE `id` = ? ;")) {
updateStatement.setLong(1, userVmId);
updateStatement.executeUpdate();
} catch (SQLException e) {
LOG.error("Failed to update cloud.user_vm user_data for id:" + userVmId + " to NULL with exception: " + e.getMessage());
throw new CloudRuntimeException("Exception while updating cloud.user_vm for id " + userVmId + " to NULL", e);
}
}
}
} catch (SQLException e) {
throw new CloudRuntimeException("Exception while validating existing user_vm table's user_data column to be base64 valid with padding", e);
}
if (LOG.isDebugEnabled()) {
LOG.debug("Done validating base64 content of user data");
}
}

@SuppressWarnings("serial")
private void updateSystemVmTemplates(final Connection conn) {
LOG.debug("Updating System Vm template IDs");
Expand Down
9 changes: 6 additions & 3 deletions server/src/com/cloud/vm/UserVmManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2525,7 +2525,7 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo
if (userData != null) {
// check and replace newlines
userData = userData.replace("\\n", "");
validateUserData(userData, httpMethod);
userData = validateUserData(userData, httpMethod);
// update userData on domain router.
updateUserdata = true;
} else {
Expand Down Expand Up @@ -3395,7 +3395,7 @@ protected UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOf
_accountMgr.checkAccess(owner, AccessType.UseEntry, false, template);

// check if the user data is correct
validateUserData(userData, httpmethod);
userData = validateUserData(userData, httpmethod);

// Find an SSH public key corresponding to the key pair name, if one is
// given
Expand Down Expand Up @@ -3942,7 +3942,7 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
}
}

private void validateUserData(String userData, HTTPMethod httpmethod) {
protected String validateUserData(String userData, HTTPMethod httpmethod) {
byte[] decodedUserData = null;
if (userData != null) {
if (!Base64.isBase64(userData)) {
Expand Down Expand Up @@ -3970,7 +3970,10 @@ private void validateUserData(String userData, HTTPMethod httpmethod) {
if (decodedUserData == null || decodedUserData.length < 1) {
throw new InvalidParameterValueException("User data is too short");
}
// Re-encode so that the '=' paddings are added if necessary since 'isBase64' does not require it, but python does on the VR.
return Base64.encodeBase64String(decodedUserData);
}
return null;
}

@Override
Expand Down
13 changes: 13 additions & 0 deletions server/test/com/cloud/vm/UserVmManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import com.cloud.event.dao.UsageEventDao;
import com.cloud.uservm.UserVm;
import org.junit.Assert;
import org.apache.cloudstack.api.BaseCmd;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
Expand Down Expand Up @@ -1056,4 +1057,16 @@ public void testPersistDeviceBusInfo() {
_userVmMgr.persistDeviceBusInfo(_vmMock, "lsilogic");
verify(_vmDao, times(1)).saveDetails(any(UserVmVO.class));
}

@Test
public void testValideBase64WithoutPadding() {
// fo should be encoded in base64 either as Zm8 or Zm8=
String encodedUserdata = "Zm8";
String encodedUserdataWithPadding = "Zm8=";

// Verify that we accept both but return the padded version
assertTrue("validate return the value with padding", encodedUserdataWithPadding.equals(_userVmMgr.validateUserData(encodedUserdata, BaseCmd.HTTPMethod.GET)));
assertTrue("validate return the value with padding", encodedUserdataWithPadding.equals(_userVmMgr.validateUserData(encodedUserdataWithPadding, BaseCmd.HTTPMethod.GET)));
}

}