Dave Chen has posted comments on this change.

Change subject: Trusted Compute Pools - Open Attestation integration with oVirt 
engine proposal
......................................................................


Patch Set 2: (39 inline comments)

....................................................
File backend/manager/dbscripts/create_tables.sql
Line 279:    migration_support INTEGER NOT NULL default 0,
Line 280:    userdefined_properties VARCHAR(4000),
Line 281:    predefined_properties VARCHAR(4000),
Line 282:    min_allocated_mem INTEGER not null default 0, --indicates how much 
memory at least VM need to run, value is in MB
Line 283:   trust_lvl VARCHAR(20)  default '',
Done
Line 284:    CONSTRAINT PK_vm_static PRIMARY KEY(vm_guid)
Line 285: ) WITH OIDS;
Line 286: 
Line 287: 


Line 279:    migration_support INTEGER NOT NULL default 0,
Line 280:    userdefined_properties VARCHAR(4000),
Line 281:    predefined_properties VARCHAR(4000),
Line 282:    min_allocated_mem INTEGER not null default 0, --indicates how much 
memory at least VM need to run, value is in MB
Line 283:   trust_lvl VARCHAR(20)  default '',
yes, I will replace "trust_lvl" with "trust_level", thanks Juan.
Line 284:    CONSTRAINT PK_vm_static PRIMARY KEY(vm_guid)
Line 285: ) WITH OIDS;
Line 286: 
Line 287: 


....................................................
File backend/manager/dbscripts/upgrade/03_02_0340_add_trust_lvl_to_vm_static.sql
Line 1: select fn_db_add_column('vm_static', 'trust_lvl', 'varchar(255) default 
''''');
agree, varchar(20) is enough to store the trustworthiness of a host. Thanks 
Juan.


....................................................
File backend/manager/modules/bll/pom.xml
Line 172:       <dependency>
Line 173:        <groupId>org.apache.httpcomponents</groupId>
Line 174:        <artifactId>httpcore</artifactId>
Line 175:        <version>4.1.2</version>
Line 176:      </dependency>
Juan, we have tried but got failure result after compiling source code, maybe 
we have not found the root cause, let's find some substituted method.
Line 177: 
Line 178:       <dependency>
Line 179:        <groupId>org.codehaus.jackson</groupId>
Line 180:        <artifactId>jackson-core-asl</artifactId>


Line 178:       <dependency>
Line 179:        <groupId>org.codehaus.jackson</groupId>
Line 180:        <artifactId>jackson-core-asl</artifactId>
Line 181:        <version>1.9.4</version>
Line 182:      </dependency>
Doron, we notice your comment in previous patch, just encounter some obstacle 
to compile source code after file changes, let me try it again. Thanks Doron.
Line 183: 
Line 184:   </dependencies>
Line 185: 
Line 186:   <build>


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationCacheManager.java
Line 2: 
Line 3: import java.util.HashMap;
Line 4: import java.util.Set;
Line 5: 
Line 6: public class AttestationCacheManager {
good example, thanks Juan.
Line 7: 
Line 8:     private static HashMap<String, AttestationValue> attestationValues 
= new HashMap<String, AttestationValue>();
Line 9:     private static long cacheTimeout = 36000000;//10h
Line 10: 


Line 4: import java.util.Set;
Line 5: 
Line 6: public class AttestationCacheManager {
Line 7: 
Line 8:     private static HashMap<String, AttestationValue> attestationValues 
= new HashMap<String, AttestationValue>();
yes, ConcurrentHashMap is better, thanks Juan.
Line 9:     private static long cacheTimeout = 36000000;//10h
Line 10: 
Line 11:     public AttestationCacheManager(){
Line 12: 


Line 5: 
Line 6: public class AttestationCacheManager {
Line 7: 
Line 8:     private static HashMap<String, AttestationValue> attestationValues 
= new HashMap<String, AttestationValue>();
Line 9:     private static long cacheTimeout = 36000000;//10h
Done
Line 10: 
Line 11:     public AttestationCacheManager(){
Line 12: 
Line 13:     }


Line 11:     public AttestationCacheManager(){
Line 12: 
Line 13:     }
Line 14: 
Line 15:     public static boolean isExisted(String key){
Done
Line 16:         return attestationValues.containsKey(key);
Line 17:     }
Line 18: 
Line 19:     public static synchronized void addToCache(AttestationValue value){


Line 17:     }
Line 18: 
Line 19:     public static synchronized void addToCache(AttestationValue value){
Line 20:         if (!attestationValues.containsKey(value.getHostName()))
Line 21:            attestationValues.put(value.getHostName(), value);
Done
Line 22:     }
Line 23: 
Line 24:     public static synchronized void removeFromCache(String key){
Line 25:         if (attestationValues.containsKey(key)){


Line 23: 
Line 24:     public static synchronized void removeFromCache(String key){
Line 25:         if (attestationValues.containsKey(key)){
Line 26:             attestationValues.remove(key);
Line 27:         }
both is okay from my point of view, but I learned from your suggestion. thanks 
Juan.
Line 28:     }
Line 29: 
Line 30:     public static AttestationValue getValueFromCache(String key){
Line 31:         AttestationValue value = attestationValues.get(key);


Line 31:         AttestationValue value = attestationValues.get(key);
Line 32:         return value;
Line 33:     }
Line 34: 
Line 35:     public static boolean IsExpired(String key){
Done
Line 36:         AttestationValue value = getValueFromCache(key);
Line 37:         if (value != null && value.getVtime() != null){
Line 38:             if (System.currentTimeMillis() - 
value.getVtime().getTime() <= cacheTimeout)
Line 39:                 return false;


Line 51:             cacheValue.setTrustLvl(value.getTrustLvl());
Line 52:             cacheValue.setVtime(value.getVtime());
Line 53:         }
Line 54:     }
Line 55: }
really good question. we will consider to clean the cache if there is no 
conflict.


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationHost.java
Line 33: import org.ovirt.engine.core.utils.log.LogFactory;
Line 34: import org.ovirt.engine.core.searchbackend.DateUtils;
Line 35: import org.ovirt.engine.core.common.businessentities.AttestationResult;
Line 36: 
Line 37: public class AttestationHost {
Done
Line 38:     private static final String POLL_URI = "PollHosts";
Line 39:     private static final String HEADER_HOSTS = "hosts";
Line 40:     private static final String HEADER_HOST_NAME = "host_name";
Line 41:     private static final String HEADER_RESULT = "trust_lvl";


Line 39:     private static final String HEADER_HOSTS = "hosts";
Line 40:     private static final String HEADER_HOST_NAME = "host_name";
Line 41:     private static final String HEADER_RESULT = "trust_lvl";
Line 42:     private static final String HEADER_VTIME = "vtime";
Line 43:     private static final int PORT = 8443;
Done
Line 44:     private static final String CONTENT_TYPE = "application/json";
Line 45: 
Line 46: 
Line 47:      public static AttestationResult validateHostIsTrusted(VDS vds) {


Line 44:     private static final String CONTENT_TYPE = "application/json";
Line 45: 
Line 46: 
Line 47:      public static AttestationResult validateHostIsTrusted(VDS vds) {
Line 48:          System.out.println("validating....");
Done
Line 49:         AttestationResult result = AttestationResult.unknown;
Line 50:         String key = vds.gethost_name();
Line 51:         if (AttestationCacheManager.isExisted(key)){
Line 52:             if (!AttestationCacheManager.IsExpired(key)){


Line 65:     public static List<AttestationValue> attestHosts(Set<String> 
hosts){
Line 66:         String attestationWSURL = Config.<String> 
GetValue(ConfigValues.AttestationWebServicesUrl);
Line 67:         DefaultHttpClient httpclient = null;
Line 68:         List<AttestationValue> values = new 
ArrayList<AttestationValue>();
Line 69:         httpclient = setupSSLConnection();
so, your suggestion is make url prefix configurable?
Line 70:         HttpPost httpPost = new HttpPost(attestationWSURL +"/" 
+POLL_URI);
Line 71:         try {
Line 72:             httpPost.setEntity(new StringEntity(writeListJson(hosts)));
Line 73:             httpPost.setHeader("Accept",CONTENT_TYPE);


Line 97:         DefaultHttpClient httpclient = new  DefaultHttpClient();
Line 98:         try {
Line 99:             URL truststoreUrl = new URL("file://" + 
Config.resolveAttestationTrustStorePath());
Line 100:              KeyStore trustStore  = 
KeyStore.getInstance(KeyStore.getDefaultType());
Line 101:              FileInputStream instream = new 
FileInputStream(truststoreUrl.getFile());
good catch, thanks Juan
Line 102:              trustStore.load(instream, null);
Line 103:              SSLSocketFactory socketFactory = new 
SSLSocketFactory(trustStore);
Line 104:              Scheme sch = new Scheme("https", PORT, socketFactory);
Line 105:              
httpclient.getConnectionManager().getSchemeRegistry().register(sch);


Line 100:              KeyStore trustStore  = 
KeyStore.getInstance(KeyStore.getDefaultType());
Line 101:              FileInputStream instream = new 
FileInputStream(truststoreUrl.getFile());
Line 102:              trustStore.load(instream, null);
Line 103:              SSLSocketFactory socketFactory = new 
SSLSocketFactory(trustStore);
Line 104:              Scheme sch = new Scheme("https", PORT, socketFactory);
Done
Line 105:              
httpclient.getConnectionManager().getSchemeRegistry().register(sch);
Line 106:              return httpclient;
Line 107:         }catch (NoSuchAlgorithmException e) {
Line 108:            log.error(e.getMessage(), e);


Line 114:             log.error(e.getMessage(), e);
Line 115:         } catch (IOException e) {
Line 116:             log.error(e.getMessage(), e);
Line 117:         }finally {
Line 118:             httpclient.getConnectionManager().shutdown();
Done
Line 119:         }
Line 120:         return httpclient;
Line 121:     }
Line 122: 


Line 124:         JsonFactory jfactory = new JsonFactory();
Line 125:         JsonParser jParser;
Line 126:         List<AttestationValue> values = new 
ArrayList<AttestationValue>();
Line 127:         try {
Line 128:             jParser = jfactory.createJsonParser(str);
agree, we miss this, thanks Juan.
Line 129:             jParser.nextToken();
Line 130:             while (jParser.nextToken() != JsonToken.END_OBJECT){
Line 131:                
if(jParser.getCurrentName().equalsIgnoreCase(HEADER_HOSTS)){
Line 132:                   while(jParser.nextToken() != JsonToken.END_ARRAY && 
jParser.getCurrentToken() != JsonToken.END_OBJECT){


Line 168:             toJsonString += "\"" +host +"\"," ;
Line 169:         }
Line 170:         toJsonString = toJsonString.substring(0, 
toJsonString.length()-1);
Line 171:         toJsonString += "]}";
Line 172:         return toJsonString;
well, StringBuffer is better, thanks.
Line 173:     }
Line 174:     private static final Log log = 
LogFactory.getLog(AttestationHost.class);


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/attestationbroker/AttestationValue.java
Line 3: 
Line 4: import org.ovirt.engine.core.common.businessentities.AttestationResult;
Line 5: 
Line 6: 
Line 7: public class AttestationValue {
yes, we miss "equals" and "hashCode" methods.
Line 8: 
Line 9:   private String hostName;
Line 10:   private AttestationResult trustLvl;
Line 11:   private Date vtime;


Line 6: 
Line 7: public class AttestationValue {
Line 8: 
Line 9:   private String hostName;
Line 10:   private AttestationResult trustLvl;
Done
Line 11:   private Date vtime;
Line 12: 
Line 13:   public AttestationValue(){
Line 14:     trustLvl = AttestationResult.unknown;


Line 7: public class AttestationValue {
Line 8: 
Line 9:   private String hostName;
Line 10:   private AttestationResult trustLvl;
Line 11:   private Date vtime;
Done
Line 12: 
Line 13:   public AttestationValue(){
Line 14:     trustLvl = AttestationResult.unknown;
Line 15:   }


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsSelector.java
Line 518:             vmNICs = 
getVmNetworkInterfaceDao().getAllForVm(getVm().getId());
Line 519:         }
Line 520:         return vmNICs;
Line 521:     }
Line 522:      private static boolean validateHostIsTrusted(VDS curVds) {
Doron, we have moved this logic to validateHostIsReadyToRun function, this 
patch is a little behind our current progress. Thanks Doron.
Line 523:         String attestationWSURL, trustStorePath;
Line 524:         attestationWSURL = Config.<String> 
GetValue(ConfigValues.AttestationWebServicesUrl);
Line 525:         trustStorePath=Config.<String> 
GetValue(ConfigValues.TrustStore);
Line 526:         DefaultHttpClient httpclient = new  DefaultHttpClient();


Line 533:         try {
Line 534:              KeyStore trustStore  = 
KeyStore.getInstance(KeyStore.getDefaultType());
Line 535:              FileInputStream instream = new FileInputStream(new 
File(trustStorePath +"/TrustStore.jks"));
Line 536:              trustStore.load(instream, null);
Line 537:              SSLSocketFactory socketFactory = new 
SSLSocketFactory(trustStore);
Doron, we can not reuse the existed function as the difference between two 
truststores, still we will try to find some work around method. Thanks Doron.
Line 538:              Scheme sch = new Scheme("https", 8443, socketFactory);
Line 539:              
httpclient.getConnectionManager().getSchemeRegistry().register(sch);
Line 540:              HttpPost httpPost = new HttpPost(attestationWSURL 
+"/PollHosts");
Line 541:              httpPost.setEntity(new StringEntity("{\"hosts\":[\"" 
+hostname +"\"]}"));


Line 561:                    }
Line 562:                   jParser.nextToken();
Line 563:                   if(jParser.getText().equalsIgnoreCase("trusted"))
Line 564:                       flag=true;
Line 565:                   System.out.println("host: " +hostname +" is " + 
jParser.getText());
Sorry, we forgot to remove this debugging code.
Line 566:               }
Line 567:           } catch (Exception e) {
Line 568:             e.printStackTrace();
Line 569:             throw new RuntimeException(e.toString());


Line 561:                    }
Line 562:                   jParser.nextToken();
Line 563:                   if(jParser.getText().equalsIgnoreCase("trusted"))
Line 564:                       flag=true;
Line 565:                   System.out.println("host: " +hostname +" is " + 
jParser.getText());
yes, this code is reserved by mistake, it's a just debugging code. we will and 
remember to remove similar code in the next time. Thanks Doron.
Line 566:               }
Line 567:           } catch (Exception e) {
Line 568:             e.printStackTrace();
Line 569:             throw new RuntimeException(e.toString());


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AttestationResult.java
Line 4: public enum AttestationResult {
Line 5:     untrusted(0),
Line 6:     trusted(1),
Line 7:     unknown(2),
Line 8:     timeout(3);
regarding to current design, all of these four cases is the result of attesting 
a host, the intention is compatible with our openattestation project, as to 
name of enum, both is okay from my side.
Line 9: 
Line 10:     private int intValue;
Line 11:     private static java.util.HashMap<Integer, AttestationResult> 
mappings = new HashMap<Integer, AttestationResult>();
Line 12: 


Line 27:     public static AttestationResult forValue(int value) {
Line 28:         return mappings.get(value);
Line 29:     }
Line 30: 
Line 31:     public static int getIntFromString(String trust_lvl){
yes, redundant code here, we will remove this method, thanks Juan.
Line 32:         int value = 2;
Line 33:         if (trust_lvl.equalsIgnoreCase("trusted"))
Line 34:             value = 0;
Line 35:         else if (trust_lvl.equalsIgnoreCase("untrusted"))


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java
Line 1464:     public String getName() {
Line 1465:         return getVmName();
Line 1466:     }
Line 1467: 
Line 1468:     public String gettrust_lvl() {
Done
Line 1469:         return vmStatic.gettrust_lvl();
Line 1470:     }
Line 1471: 
Line 1472:     public void settrust_lvl(String trust_lvl) {


Line 1468:     public String gettrust_lvl() {
Line 1469:         return vmStatic.gettrust_lvl();
Line 1470:     }
Line 1471: 
Line 1472:     public void settrust_lvl(String trust_lvl) {
Done
Line 1473:         vmStatic.settrust_lvl(trust_lvl);
Line 1474:     }
Line 1475:     
Line 1476: 


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java
Line 68:     @Column(name = "host_cpu_flags", nullable = false)
Line 69:     private boolean useHostCpuFlags = false;
Line 70: 
Line 71:     @Column(name = "trust_lvl")
Line 72:     private String trust_lvl="";
Done
Line 73: 
Line 74:     public VmStatic() {
Line 75:         setNumOfMonitors(1);
Line 76:         initialized = false;


Line 78:         setNiceLevel(0);
Line 79:         setDefaultBootSequence(BootSequence.C);
Line 80:         defaultDisplayType = DisplayType.qxl;
Line 81:         setVmType(VmType.Desktop);
Line 82:         trust_lvl="trusted";
Done
Line 83:     }
Line 84: 
Line 85:     public VmStatic(VmStatic vmStatic) {
Line 86:         super(vmStatic.getId(),


Line 254:     public void setCpuPinning(String cpuPinning) {
Line 255:         this.cpuPinning = cpuPinning;
Line 256:     }
Line 257: 
Line 258:    public String gettrust_lvl() {
Done
Line 259:         return trust_lvl;
Line 260:     }
Line 261: 
Line 262:     public void settrust_lvl(String trust_lvl) {


Line 258:    public String gettrust_lvl() {
Line 259:         return trust_lvl;
Line 260:     }
Line 261: 
Line 262:     public void settrust_lvl(String trust_lvl) {
Done
Line 263:         this.trust_lvl = trust_lvl;
Line 264:     }
Line 265: 
Line 266:     @Override


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1337:     AttestationWebServicesUrl(411),
Line 1338: 
Line 1339:     @TypeConverterAttribute(String.class)
Line 1340:     @DefaultValueAttribute("/etc/pki/ovirt-engine/certs")
Line 1341:     TrustStore(412),
yes, we can not reuse TruststoreUrl beacuse the certificate stored in 
TruststoreUrl directory is not we need, we're thinking is there any work around 
method to solve this problem, thanks Doron.
Line 1342: 
Line 1343:     Invalid(65535);
Line 1344: 
Line 1345:     private int intValue;


....................................................
File 
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAODbFacadeImpl.java
Line 83:                 .addValue("quota_id", vm.getQuotaId())
Line 84:                 .addValue("allow_console_reconnect", 
vm.isAllowConsoleReconnect())
Line 85:                 .addValue("cpu_pinning", vm.getCpuPinning())
Line 86:                 .addValue("host_cpu_flags", vm.isUseHostCpuFlags())
Line 87:                 .addValue("trust_lvl", vm.gettrust_lvl());
yes, we have added to VMStaticRowMapper, but we have not verifed this changes 
in this patchset due to tight schedule, we will comprise this change in the 
next patchset, thanks Doron, learning from your responsibility :)
Line 88:     }
Line 89: 
Line 90:     @Override
Line 91:     public void remove(Guid id) {


--
To view, visit http://gerrit.ovirt.org/11237
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de780cd46069638433255f3f9c994575f752e55
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dave Chen <wei.d.c...@intel.com>
Gerrit-Reviewer: Dave Chen <wei.d.c...@intel.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to